Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/i10 pytests #65

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix/i10 pytests #65

wants to merge 7 commits into from

Conversation

99alfie
Copy link
Collaborator

@99alfie 99alfie commented Aug 21, 2023

bugs in pytests resolved, all working now

@99alfie 99alfie linked an issue Aug 21, 2023 that may be closed by this pull request
@99alfie 99alfie requested a review from salvaom August 21, 2023 12:10
assert users_split[0].first_name == [initial_operations[1]["first_name"]]
cached_users = SESSION.get_cached_collections()[User]
for cached_user in cached_users:
user = Query(User).by_id(cached_user.id[0]).get_first(projections=["username", "first_name"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the purpose of the cache that we won't have to query again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not write the test but I guess the query is to make sure the user actually exists and comparing it with the user from "initial_operations". If you compare my changes to what it was before, I did not actually change anything in the test itself, just added an additional check for the user's existence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean you took over the existing changes from @salvaom?

I'm still wondering why the Query call has been added because for me it defeats the purpose of this test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the query is added as an additional check for the user's existence. get_cached_collections returns two user ids, the first one of which does not exist and will cause the test to fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, the user shouldn't be taken from the query result but from the cache when it exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that you don't pass the session object to the query. Remember that Query is taking the SESSION singleton instance by default.

Let me know if this solves the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, that solves the problem for test_deferred_operations.

Still getting that "mystery user" - so I just saw that there are dbm files checked in under tests/resources/session - which when deleted makes all the tests fail (and which probably contain that nonexisting user).
This dbm file should be created by the test and deleted thereafter for the test to be valid, which I think was done in the original test_session.py before trackteroid.

In order to proceed, I need to understand how the original tests were working. Since it is not quite obvious in the code, I woud need some clarifications here:

import anydbm
import os
import logging
import tempfile
import shutil
import uuid

from trixter.ftrack import (
    Query,
    SESSION,
)

from trixter.ftrack.session import (
    CreateEntityOperation,
    UpdateEntityOperation
)

from trixter.ftrack.entities import *

from .base import TestCase

_LOG = logging.getLogger("tx.test.ftrack.session")


class TestSessionDelegate(TestCase):
    auto_populate = False
    server_url = "http://ftrack-devel.trixter.intern"

    @classmethod
    def setUpClass(cls):
        super(TestSessionDelegate, cls).setUpClass()
        cls.tmp_dir = os.path.join(tempfile.gettempdir(), str(uuid.uuid4()))

        os.makedirs(cls.tmp_dir)

        cls.dumped_operations_file = os.path.join(cls.tmp_dir, "operations.dbm")
        cls.initial_operations_file = os.path.join(os.path.dirname(__file__), "fixtures", "operations.dbm")

    @classmethod
    def tearDownClass(cls):
        shutil.rmtree(cls.tmp_dir)

    @classmethod
    def setUp(cls):
        Query(AssetVersion).by_id("fbb682b6-e9e6-4111-8edb-38d0797c9ffe").get_first().delete().commit()

Where is that assetversion coming from? Why is it deleted on "setup"

    def test_deferred_operations(self):

        # case 1. clear an existing cache temporarily
        query_result = self.samplebag.grab("AssetVersion", attributes=["task", "version"])
        operations_count = len(SESSION.recorded_operations)

        # do a query -> cache a result
        with SESSION.deferred_operations(self.dumped_operations_file):
            self.assertEqual(0, len(SESSION.recorded_operations))

            avs = Query(AssetVersion).by_id(query_result["id"]).get_one(projections=["task"])
            avs.version = avs.version[0] + 10
            avs2 = avs.create(task=avs.task)  # -> create entity, update task, update asset
            avs2.version = avs.version[0] + 10
            
            self.assertEqual(5, len(SESSION.recorded_operations))

        self.assertEqual(operations_count, len(SESSION.recorded_operations))
        # check the created file database
        database = anydbm.open(self.dumped_operations_file, "r")
        def make_keys(entity_collection):
            return entity_collection.map(
                lambda x: "('{}', ['{}'])".format(x.entity_type.__name__, x.id[0])
            )

        expected_keys = make_keys(avs.union(avs2)) + make_keys(avs.task) + make_keys(avs.asset) + ["__operations__"]
        self.assertItemsEqual(expected_keys, database.keys())

    def test_reconnect_and_commit(self):

        SESSION.reconnect_and_commit(
            self.initial_operations_file,
            server_url=self.server_url
        )

        assetversion = Query(AssetVersion).by_id("fbb682b6-e9e6-4111-8edb-38d0797c9ffe").get_one(
            projections=["components.name"]
        )
        self.assertEqual(99, assetversion.version[0])
        self.assertEqual("pymelle", assetversion.components.name[0])

Again, where/when is that Assetversion/Component created?

    def test_get_cached_collections(self):

        SESSION.reconnect_and_commit(
            self.initial_operations_file,
            server_url=self.server_url
        )

        avs = SESSION.get_cached_collections()[AssetVersion]
        avs.fetch_attributes("asset.versions")

        v99, rest = avs.asset.versions.partition(lambda x: x.version[0] == 99)
        v99.uses_versions = rest

        SESSION.commit()
        ```

Also, I do not see code that writes the dbm file with certain operations. Where/how does that happen?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still getting that "mystery user" - so I just saw that there are dbm files checked in under tests/resources/session - which when deleted makes all the tests fail (and which probably contain that nonexisting user).

Yes, the dbm file is the fixture/dataset that has to correlate with expectations in the test code. So you shouldn't delete them and either stick to the operations this database contains (the user creation) or regenerate other dbm files with your relevant changes that correlate with your adjustments. Please also note that the dbm files are different for linux and windows, which is a requirement for cross-platform testing.

Be aware that it is not trivial to change the dataset as many operations rely on parent ids that are not reproducible across test runs. This was one problem I stumbled on and found the User to be an entity which serves our test purpose quite well.

This dbm file should be created by the test and deleted thereafter for the test to be valid, which I think was done in the original test_session.py before trackteroid.

What tests are you referring to exactly? From what I can see in the code it is how it should be:

We have initial operations which is a pre-dumped file that contains the operations we test against.

And then we have deferred operations that is a temporary file path and these database files will be created by some tests itself.

Pretty identical to the original test cases you've pasted:

        cls.dumped_operations_file = os.path.join(cls.tmp_dir, "operations.dbm")
        cls.initial_operations_file = os.path.join(os.path.dirname(__file__), "fixtures", "operations.dbm")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, so the dbm file needs to be pre-created and is not created as part of the tests, that clarifies it, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, so the dbm file needs to be pre-created and is not created as part of the tests, that clarifies it, thank you.

Yes, for what we call initial operations.

@99alfie
Copy link
Collaborator Author

99alfie commented Sep 4, 2023

Sure, I checked in the current state to the fix/i10-pytest branch

@rkoschmitzky
Copy link
Contributor

@99alfie @salvaom I just want to verify if there's any further information needed from my side to proceed with merging the updated tests.

@99alfie
Copy link
Collaborator Author

99alfie commented Oct 4, 2023

Hi @rkoschmitzky thank you for asking, I am just a little delayed with the implementation but with your help I now know what to do. Just takes some time. But no issues atm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port and rework tests
3 participants