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

LIU-407: Use EAGLE_test_repo as the basis for test graphs #287

Merged
merged 7 commits into from
Oct 8, 2024
Merged

Conversation

myxie
Copy link
Collaborator

@myxie myxie commented Oct 1, 2024

(Note: This relies on files introduced in #278, which is why this is currently targetting that branch. This must be merged after that PR is merged, at which point this should be re-targetted to master. It is probably a good idea to wait for EAGLE test repo PR #2 to merge, too.)

Issue

This PR addresses LIU-407, which aims to create a standardised graph repository for DALiuGE/EAGLE. When we make changes are made to the graph schema/EAGLE graph output, we need to:

a) Update some code in the translator/engine based on those changes, and
b) (Sometimes) update the graphs that are used in the unittests, depending on the changes to the input/translated graphs.

The problem with this is not all unit tests fail when updates occur, and not all unit-tests cover all graphs. This means some graphs will be updated more regularly (e.g. for the translator) and other graphs not regularly (e.g. engine test graphs).

The result of this is that we don't effectively synchronize our graphs to the EAGLE output, and we also have to go through >80 graphs individually, open them in EAGLE, and then save them again, to ensure our tests are accurately testing the correct graph formats. Hence, having all the test graphs in one place, that can be updated more easily and is separate to the DALiuGE and EAGLE repos will improve the consistency of our testing moving forward. It should also help us version and release the schema effectively.

Solution

This PR removes the existing test graphs from DALiuGE, and redirects all the tests cases to the daliuge_tests module that is installed when we install the eagle-test-repo project (established in the corresponding PR in the EAGLE_test_repo).

Now, we install the eagle-test-repo when running a test install of DALiuGE:

$ pip install -e ".[test]"

It is then possible to access the test graphs in the daliuge_tests module:

import daliuge_tests.reproGraphs as test_graphs
from importlib.resources import files

workflow = "apps.graph"
lg_path  = files(test_graphs) / workflow

As a side-effect of these changes, I've also transitioned the unittests away from the deprecated pkg_resources towards importlib.resources

@coveralls
Copy link

Coverage Status

coverage: 79.627%. remained the same
when pulling 275415d on LIU-407
into 090301a on LIU-402.

- Remove the test-graphs from daliuge-engine and daliuge-translator
- Update the unittests to retrieve the graphs from the installed daliuge_tests from eagle-test-repo.

This also has served as a good opportunity to transition a lot of our tests to using importlib.resources, and remove our reliance on the deprecated pkg_resources.
@myxie myxie changed the title (WIP) LIU-407: Use EAGLE_test_repo as the basis for test graphs LIU-407: Use EAGLE_test_repo as the basis for test graphs Oct 3, 2024
@myxie myxie marked this pull request as ready for review October 3, 2024 09:13
@myxie myxie requested a review from awicenec October 3, 2024 09:22
Copy link
Contributor

@awicenec awicenec left a comment

Choose a reason for hiding this comment

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

This is an excellent update to the test environment, thanks!

Base automatically changed from LIU-402 to master October 8, 2024 06:47
@myxie myxie merged commit d075337 into master Oct 8, 2024
17 checks passed
@myxie myxie deleted the LIU-407 branch October 8, 2024 08:07
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.

3 participants