test_chgrp_new_container[user] might have a race condition

[pwalczysko]

test_chgrp_new_container[user] OmeroWeb.test.integration.test_chgrp.TestChgrp

which lives in

openmicroscopy/components/tools/OmeroWeb/test/integration/test_chgrp.py

Line 198 in bf5e0bd

def test_chgrp_new_container(self, dataset, credentials):

might have a race condition.

See its failure which happened today on Jenkins merge-ci below.

Initial investigation shows that the test

• when run within the full suite in all cases is sometimes failing and sometimes passing (more passing than failing
• when run singly it is passing, even though run in the same env which was just used by the failing test above
• it might be relying on a fast return of the server-side chgrp call before calling the getAllObjects() which causes a race condition

cc @will-moore

Failure in https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/16/testReport/OmeroWeb.test.integration.test_chgrp/TestChgrp/test_chgrp_new_container_user_/ (copied in full below in case we lose the Jenkins record):

AssertionError: assert {'update': {'...ned': False}}} == {'update': {'...e': [], ...}}}
  
  Differing items:
  {'update': {'remove': {'project': [], 'dataset': [1248], 'screen': [], 'plate': [], ...}, 'childless': {'project': [994], 'dataset': [], 'orphaned': False}}} != {'update': {'childless': {'project': [], 'orphaned': False, 'dataset': []}, 'remove': {'project': [], 'plate': [], 'screen': [], 'image': [], ...}}}
  
  Full diff:
    {
        'update': {
  -         'childless': {
  -             'project': [],
  -             'orphaned': False,
  -             'dataset': [],
  -         },
            'remove': {
                'project': [],
  -             'plate': [],
  -             'screen': [],
  -             'image': [],
                'dataset': [
                    1248,
                ],
  +             'screen': [],
  +             'plate': [],
  +             'image': [],
  +         },
  +         'childless': {
  +             'project': [
  +                 994,
  +             ],
  +             'dataset': [],
  +             'orphaned': False,
            },
        },
    }

Stack Trace

self = <test_chgrp.TestChgrp object at 0x7f15bb732750>
dataset = object #0 (::omero::model::Dataset)
{
    _id = object #1 (::omero::RLong)
    {
        _val = 1248
    }
    _detail...bject #54 (::omero::RString)
    {
        _val = 38faf46a-510b-4b3e-aae9-d1074fbfb645
    }
    _description = <nil>
}
credentials = 'user'

    @pytest.mark.parametrize("credentials", ['user', 'admin'])
    def test_chgrp_new_container(self, dataset, credentials):
        """
        Performs a chgrp POST, polls the activities json till done,
        then checks that Dataset has moved to new group and has new
        Project as parent.
        """
    
        django_client = self.get_django_client(credentials)
        request_url = reverse('chgrp')
        projectName = "chgrp-project%s" % (self.uuid())
        data = {
            "group_id": self.group2.id.val,
            "Dataset": dataset.id.val,
            "new_container_name": projectName,
            "new_container_type": "project",
        }
        rsp = post(django_client, request_url, data)
        data = json.loads(rsp.content)
        expected = {"update": {"childless": {"project": [],
                                             "orphaned": False,
                                             "dataset": []},
                               "remove": {"project": [],
                                          "plate": [],
                                          "screen": [],
                                          "image": [],
                                          "dataset": [dataset.id.val]}}}
>       assert data == expected
E       AssertionError: assert {'update': {'...ned': False}}} == {'update': {'...e': [], ...}}}
E         
E         Differing items:
E         {'update': {'remove': {'project': [], 'dataset': [1248], 'screen': [], 'plate': [], ...}, 'childless': {'project': [994], 'dataset': [], 'orphaned': False}}} != {'update': {'childless': {'project': [], 'orphaned': False, 'dataset': []}, 'remove': {'project': [], 'plate': [], 'screen': [], 'image': [], ...}}}
E         
E         Full diff:
E           {
E               'update': {
E         -         'childless': {
E         -             'project': [],
E         -             'orphaned': False,
E         -             'dataset': [],
E         -         },
E                   'remove': {
E                       'project': [],
E         -             'plate': [],
E         -             'screen': [],
E         -             'image': [],
E                       'dataset': [
E                           1248,
E                       ],
E         +             'screen': [],
E         +             'plate': [],
E         +             'image': [],
E         +         },
E         +         'childless': {
E         +             'project': [
E         +                 994,
E         +             ],
E         +             'dataset': [],
E         +             'orphaned': False,
E                   },
E               },
E           }

test/integration/test_chgrp.py:224: AssertionError

[will-moore]

As far as I can see (reading diffs of JSON is hard), the test is failing because we are getting unexpected:

"update": {"childless": {"project": [PID]}}

Looking at the code, the "childless" project is found by querying for Parents of the Dataset that is in the POST request.
It is expected that this query runs before the chgrp happens - the response is designed to tell the webclient that "project": [ID] (that contained the specified Dataset) will become "childless" after the chgrp, so that the webclient can update jsTree.

However, it seems the test is failing because the chgrp happens before the querying for parents of the Dataset. This means that the Dataset has already been moved into the new Project, and this parent Project is then returned.

This probably works fine for most of the time because the omero-web code kicks-off the async chgrp process (which takes some time) and immediately does the parent query. Normally this would be the same as doing the parent query FIRST, but if the chgrp is practically instant then we get an unexpected result.

The fix is to actually do the parent query FIRST, and then to kick-off the chgrp process.
I'll open a PR in omero-web...