#437 Add tests for 'continue' part of start_next_batch and component resubmit
Merged 7 years ago by jkaluza. Opened 7 years ago by jkaluza.
jkaluza/fm-orchestrator fix-tests  into  master

file modified
+1
@@ -155,6 +155,7 @@ 

  

      KOJI_CONFIG = './conf/koji.conf'

      KOJI_PROFILE = 'staging'

+     SERVER_NAME = 'localhost'

  

  

  class ProdConfiguration(BaseConfiguration):

@@ -59,21 +59,6 @@ 

  

  db = SQLAlchemy(app)

  

- 

I'm just moving get_url_for under the logging intialization to be able to show debug message if the app_context issue hits someone else in the future.

- def get_url_for(*args, **kwargs):

-     """

-     flask.url_for wrapper which creates the app_context on-the-fly.

-     """

-     if has_app_context():

-         return url_for(*args, **kwargs)

- 

-     # Localhost is right URL only when the scheduler runs on the same

-     # system as the web views.

-     app.config['SERVER_NAME'] = 'localhost'

-     with app.app_context():

-         return url_for(*args, **kwargs)

- 

- 

  @app.errorhandler(ValidationError)

  def validationerror_error(e):

      """Flask error handler for ValidationError exceptions"""
@@ -118,4 +103,20 @@ 

  init_logging(conf)

  log = getLogger(__name__)

  

+ def get_url_for(*args, **kwargs):

+     """

+     flask.url_for wrapper which creates the app_context on-the-fly.

+     """

+     if has_app_context():

+         return url_for(*args, **kwargs)

+ 

+     # Localhost is right URL only when the scheduler runs on the same

+     # system as the web views.

+     app.config['SERVER_NAME'] = 'localhost'

+     with app.app_context():

+         log.debug("WARNING: get_url_for() has been called without the Flask "

+             "app_context. That can lead to SQLAlchemy errors caused by "

+             "multiple session being used in the same time.")

+         return url_for(*args, **kwargs)

+ 

  from module_build_service import views

@@ -524,6 +524,8 @@ 

                  raise UnprocessableEntity(err_msg)

  

  def record_component_builds(scm, mmd, module, initial_batch = 1):

+     import koji  # Placed here to avoid py2/py3 conflicts...

+ 

      # Format the modulemd by putting in defaults and replacing streams that

      # are branches with commit hashes

      try:
@@ -574,10 +576,10 @@ 

  

              existing_build = models.ComponentBuild.query.filter_by(

                  module_id=module.id, package=pkg.name).first()

-             if existing_build and \

-                     existing_build.state != models.BUILD_STATES['done']:

-                 existing_build.state = models.BUILD_STATES['init']

-                 db.session.add(existing_build)

+             if existing_build:

+                 if existing_build.state != koji.BUILD_STATES['COMPLETE']:

+                     existing_build.state = None

+                     db.session.add(existing_build)

              else:

                  # XXX: what about components that were present in previous

                  # builds but are gone now (component reduction)?
@@ -625,7 +627,9 @@ 

              raise Conflict(err_msg)

          log.debug('Resuming existing module build %r' % module)

          module.username = username

-         module.transition(conf, models.BUILD_STATES["init"])

+         module.transition(conf, models.BUILD_STATES["init"],

+                           "Resubmitted by %s" % username)

+         module.batch = 0

          log.info("Resumed existing module build in previous state %s"

                   % module.state)

      else:

file modified
+1 -1
@@ -242,7 +242,7 @@ 

          session.commit()

  

  

- def test_resuse_component_init_data():

+ def test_reuse_component_init_data():

      db.session.remove()

      db.drop_all()

      db.create_all()

@@ -0,0 +1,36 @@ 

+ document: modulemd

+ version: 1

+ data:

+     summary: A test module in all its beauty

+     description: This module demonstrates how to write simple modulemd files And can be used for testing the build and release pipeline.

+     license:

+         module: [ MIT ]

+     dependencies:

+         buildrequires:

+             bootstrap: master

+         requires:

+             bootstrap: master

+     references:

+         community: https://fedoraproject.org/wiki/Modularity

+         documentation: https://fedoraproject.org/wiki/Fedora_Packaging_Guidelines_for_Modules

+         tracker: https://taiga.fedorainfracloud.org/project/modularity

+     profiles:

+         default:

+             rpms:

+                 - tangerine

+     api:

+         rpms:

+             - perl-Tangerine

+             - tangerine

+     components:

+         rpms:

+             perl-List-Compare:

+                 rationale: A dependency of tangerine.

+                 ref: f25

+             perl-Tangerine:

+                 rationale: Provides API for this module and is a dependency of tangerine.

+                 ref: f25

+             tangerine:

+                 rationale: Provides API for this module.

+                 buildorder: 10

+                 ref: f25

file modified
+114 -7
@@ -19,7 +19,8 @@ 

  # SOFTWARE.

  

  import unittest

- from os import path

+ from os import path, mkdir

+ from shutil import copyfile

  import vcr

  import modulemd

  from mock import patch
@@ -27,20 +28,52 @@ 

  import module_build_service.scm

  from module_build_service import models, conf

  from module_build_service.errors import ProgrammingError, ValidationError

- from tests import test_resuse_component_init_data, init_data, db

+ from tests import test_reuse_component_init_data, init_data, db

  import mock

  from mock import PropertyMock

  import koji

  import module_build_service.scheduler.handlers.components

  from module_build_service.builder import GenericBuilder, KojiModuleBuilder

+ from tests import app

  

  BASE_DIR = path.abspath(path.dirname(__file__))

  CASSETTES_DIR = path.join(

      path.abspath(path.dirname(__file__)), '..', 'vcr-request-data')

  

+ class MockedSCM(object):

+     def __init__(self, mocked_scm, name, mmd_filename, commit=None):

+         self.mocked_scm = mocked_scm

+         self.name = name

+         self.commit = commit

+         self.mmd_filename = mmd_filename

+ 

+         self.mocked_scm.return_value.checkout = self.checkout

+         self.mocked_scm.return_value.name = self.name

+         self.mocked_scm.return_value.branch = 'master'

+         self.mocked_scm.return_value.get_latest = self.get_latest

+         self.mocked_scm.return_value.commit = self.commit

+         self.mocked_scm.return_value.repository_root = "git://pkgs.stg.fedoraproject.org/modules/"

+ 

+     def checkout(self, temp_dir):

+         scm_dir = path.join(temp_dir, self.name)

+         mkdir(scm_dir)

+         base_dir = path.abspath(path.dirname(__file__))

+         copyfile(path.join(base_dir, '..', 'staged_data', self.mmd_filename),

+                     path.join(scm_dir, self.name + ".yaml"))

+ 

+         return scm_dir

+ 

+     def get_latest(self, branch = 'master'):

+         return self.commit if self.commit else branch

  

  class TestUtils(unittest.TestCase):

  

+     def setUp(self):

+         pass

+ 

+     def tearDown(self):

+         init_data()

+ 

      @vcr.use_cassette(

          path.join(CASSETTES_DIR, 'tests.test_utils.TestUtils.test_format_mmd'))

      @patch('module_build_service.scm.SCM')
@@ -83,14 +116,14 @@ 

          self.assertEqual(mmd.xmd, xmd)

  

      def test_get_reusable_component_same(self):

-         test_resuse_component_init_data()

+         test_reuse_component_init_data()

          new_module = models.ModuleBuild.query.filter_by(id=2).one()

          rv = module_build_service.utils.get_reusable_component(

              db.session, new_module, 'tangerine')

          self.assertEqual(rv.package, 'tangerine')

  

      def test_get_reusable_component_different_perl_tangerine(self):

-         test_resuse_component_init_data()

+         test_reuse_component_init_data()

          second_module_build = models.ModuleBuild.query.filter_by(id=2).one()

          mmd = second_module_build.mmd()

          mmd.components.rpms['perl-Tangerine'].ref = \
@@ -120,7 +153,7 @@ 

          self.assertEqual(tangerine_rv, None)

  

      def test_get_reusable_component_different_buildrequires_hash(self):

-         test_resuse_component_init_data()

+         test_reuse_component_init_data()

          second_module_build = models.ModuleBuild.query.filter_by(id=2).one()

          mmd = second_module_build.mmd()

          mmd.xmd['mbs']['buildrequires']['base-runtime']['ref'] = \
@@ -144,7 +177,7 @@ 

          self.assertEqual(tangerine_rv, None)

  

      def test_get_reusable_component_different_buildrequires(self):

-         test_resuse_component_init_data()

+         test_reuse_component_init_data()

          second_module_build = models.ModuleBuild.query.filter_by(id=2).one()

          mmd = second_module_build.mmd()

          mmd.buildrequires = {'some_module': 'master'}
@@ -284,6 +317,43 @@ 

  

          self.assertTrue(str(cm.exception).endswith(' No value provided.'))

  

+     @patch('module_build_service.scm.SCM')

+     def test_resubmit(self, mocked_scm):

+         """

+         Tests that the module resubmit reintializes the module state and

+         component states properly.

+         """

+         MockedSCM(mocked_scm, 'testmodule', 'testmodule-bootstrap.yaml',

+                         '620ec77321b2ea7b0d67d82992dda3e1d67055b4')

+         with app.app_context():

+             test_reuse_component_init_data()

+             # Mark the module build as failed, so we can resubmit it.

+             module_build = models.ModuleBuild.query.filter_by(id=2).one()

+             module_build.batch = 2

+             module_build.state = models.BUILD_STATES['failed']

+             module_build.state_reason = "Cancelled"

+             module_build.version = 1

+ 

+             # Mark the components as COMPLETE/FAILED/CANCELED

+             components = module_build.component_builds

+             complete_component = components[0]

+             complete_component.state = koji.BUILD_STATES['COMPLETE']

+             failed_component = components[1]

+             failed_component.state = koji.BUILD_STATES['FAILED']

+             canceled_component = components[2]

+             canceled_component.state = koji.BUILD_STATES['CANCELED']

+             db.session.commit()

+ 

+             module_build_service.utils.submit_module_build_from_scm(

+                 "Tom Brady", 'git://pkgs.stg.fedoraproject.org/modules/testmodule.git?#8fea453',

+                 'master')

+ 

+             self.assertEqual(module_build.state, models.BUILD_STATES['wait'])

+             self.assertEqual(module_build.batch, 0)

+             self.assertEqual(module_build.state_reason, "Resubmitted by Tom Brady")

+             self.assertEqual(complete_component.state, koji.BUILD_STATES['COMPLETE'])

+             self.assertEqual(failed_component.state, None)

+             self.assertEqual(canceled_component.state, None)

  

  class DummyModuleBuilder(GenericBuilder):

      """
@@ -351,7 +421,7 @@ 

  class TestBatches(unittest.TestCase):

  

      def setUp(self):

-         test_resuse_component_init_data()

+         test_reuse_component_init_data()

          GenericBuilder.register_backend_class(DummyModuleBuilder)

  

      def tearDown(self):
@@ -360,6 +430,15 @@ 

          GenericBuilder.register_backend_class(KojiModuleBuilder)

  

      def test_start_next_batch_build_reuse(self, default_buildroot_groups):

+         """

+         Tests that start_next_batch_build:

+            1) Increments module.batch.

+            2) Can reuse all components in batch

+            3) Returns proper further_work messages for reused components.

+            4) Returns the fake Repo change message

+            5) Handling the further_work messages lead to proper tagging of

+               reused components.

+         """

          module_build = models.ModuleBuild.query.filter_by(id=2).one()

          module_build.batch = 1

  
@@ -393,3 +472,31 @@ 

  

          # Check that packages have been tagged just once.

          self.assertEqual(len(DummyModuleBuilder.TAGGED_COMPONENTS), 2)

+ 

+     def test_start_next_batch_continue(self, default_buildroot_groups):

+         """

+         Tests that start_next_batch_build does not start new batch when

+         there are unbuilt components in the current one.

+         """

+         module_build = models.ModuleBuild.query.filter_by(id=2).one()

+         module_build.batch = 2

+ 

+         # Mark the component as BUILDING.

+         building_component = module_build.current_batch()[0]

+         building_component.state = koji.BUILD_STATES['BUILDING']

+         db.session.commit()

+ 

+         builder = mock.MagicMock()

+         further_work = module_build_service.utils.start_next_batch_build(

+             conf, module_build, db.session, builder)

+ 

+         # Batch number should not increase.

+         self.assertEqual(module_build.batch, 2)

+ 

+         # Single component should be reused this time, second message is fake

+         # KojiRepoChange.

+         self.assertEqual(len(further_work), 2)

+         self.assertEqual(further_work[0].build_name, "perl-List-Compare")

+ 

+ 

+ 

Fun with get_url_for, SQLAlchemy and app_context included.

ModuleBuild.json() calls get_url_for() which creates the app_context if it does not exist. That works quite well unless you decide to do some changes to DB object before the get_url_for gets called. If you do so, those objects are associated with default db.session, but it seems that creation of app_context (even with the with app.app_context(): statement) somehow switches the default db.session internals, so the objects you have edited now belongs to different (old) DB session. This leads to errors like:

Parent instance <ModuleBuild at 0x7f641828ce10> is not bound to a Session; lazy load operation of attribute 'component_builds' cannot proceed

The solution seems to be to create app_context in tests before you touch the DB for first time and use that app_context for whole time you are expecting the DB to work.

I'm just moving get_url_for under the logging intialization to be able to show debug message if the app_context issue hits someone else in the future.

rebased

7 years ago

1 new commit added

  • Add missing testmodule-bootstrap.yaml
7 years ago

Can you put this value in the TestConfiguration class:
https://pagure.io/fm-orchestrator/blob/master/f/conf/config.py#_144

Shouldn't this be in the setUp function?

This is okay for now but I'd prefer if we consolidated the number of MockedSCM classes to a single one that all tests share.

Not 100% sure but I think get_latest is supposed to return the latest commit hash

Oops, seems like I had an extra "s" in there when I wrote that function. :disappointed:

I think why get_url_for is causing you problems is that it's using the app object from __init__.py and not the one created in the tests module:
https://pagure.io/fm-orchestrator/blob/master/f/module_build_service/__init__.py#_73

Some minor comments, but +1 once you address them.

It is there to reinitialize the database after the test_reuse_component_init_data(). We could either put init_data() to setUp() of every test class to be user it has the right data, or presume that database is always initialized in init_data phase and if we change it, reinitialize it. For now I'm doing the second way.

I think why get_url_for is causing you problems is that it's using the app object from init.py and not the one created in the tests module:
https://pagure.io/fm-orchestrator/blob/master/f/module_build_service/__init__.py#_73

init.py in tests is importing module_build_service.app, which is the same "app" instance as the one you are linking to above, isn't it?

Even if that would be a true, I would think that "with app.app_context():" should not influence things outside of with statement.

1 new commit added

  • Fix typo in test_reuse_component_init_data name, set SERVER_NAME config variable in config.py, return commit in MockedSCM.get_latest
7 years ago

1 new commit added

  • Reset batch counter and state_reason when resubmiting module
7 years ago

I think why get_url_for is causing you problems is that it's using the app object from init.py and not the one created in the tests module:
https://pagure.io/fm-orchestrator/blob/master/f/module_build_service/__init__.py#_73

init.py in tests is importing module_build_service.app, which is the same "app" instance as the one you are linking to above, isn't it?
Even if that would be a true, I would think that "with app.app_context():" should not influence things outside of with statement.

After doing some tests it seems that you are right. They are the same objects as global config changes affect them both. I'll keep reading about the Flask application context, but you're work around is totally fine.

+1 to the new changes.

Pull-Request has been merged by jkaluza

7 years ago