LIU-388: Update test_pg_gen.py to compare results to test data.

[myxie]

Issue

There exists a number of daliuge-translator that do not assert anything in the test cases; their only currently use-case is to ensure that we don't cause run-time errors when updating the code, rather than checking for specific regressions.

def test_pg_test(self):
  fp = get_lg_fname("test_grpby_gather.graph")
  lg = LG(fp)
  lg.unroll_to_tpl()

This leaves me quite worried about the potential of introducing regressions when updating the code in lg.py/pgt(p).py, as we are not doing regression testing with our tests.

Solution

This PR introduces some assert-based integration tests to confirm that we are not regressing, or to notify us to to update the existing test graphs in the event of a schema change.

Summary by Sourcery

This pull request enhances the test_pg_gen.py file by adding assert-based integration tests to ensure regression testing for logical graph unrolling and physical graph template generation. It introduces new test data files in JSON and pickle formats and includes comprehensive tests for various partitioning methods and their results.

• Enhancements:
  • Added assert-based integration tests to ensure regression testing for logical graph unrolling and physical graph template generation.
  • Introduced new test data files in JSON and pickle formats for validating logical graph unrolling and physical graph template generation.
  • Enhanced test_pg_gen.py to include detailed regression tests for various partitioning methods and their results.
• Tests:
  • Added comprehensive tests for LG unrolling, PGT to JSON conversion, and various partitioning methods including MetisPGTP, MySarkarPGTP, and MinNumPartsPGTP.
  • Included regression tests to confirm that partitioning and generating PGT specs work correctly with multiple nodes and data islands.

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request addresses the lack of assert-based integration tests in the daliuge-translator repository. The changes introduce regression tests to ensure that updates to the code do not introduce regressions. The tests compare the results of logical graph unrolling and physical graph template generation against pre-generated test data.

File-Level Changes

Files

Changes

daliuge-translator/test/dropmake/pg_spec/HelloWorld_simple.json daliuge-translator/test/dropmake/pg_spec/Plasma_test.json daliuge-translator/test/dropmake/pg_spec/SharedMemoryTest_update.json daliuge-translator/test/dropmake/pg_spec/chiles_simple.json daliuge-translator/test/dropmake/pg_spec/cont_img_mvp.json daliuge-translator/test/dropmake/pg_spec/eagle_gather_empty_update.json daliuge-translator/test/dropmake/pg_spec/eagle_gather_simple_update.json daliuge-translator/test/dropmake/pg_spec/eagle_gather_update.json daliuge-translator/test/dropmake/pg_spec/testLoop.json daliuge-translator/test/dropmake/pg_spec/test_grpby_gather.json

Added JSON files containing expected physical graph templates for various logical graphs.

[sourcery-ai]

Hey @myxie - I've reviewed your changes and they look great!

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🟢 Security: all looks good
• 🟡 Testing: 9 issues found
• 🟢 Complexity: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

[sourcery-ai]

suggestion (testing): Add edge case for empty graph

Consider adding a test case for an empty graph to ensure that the unroll_to_tpl method handles it gracefully.

Suggested change

	class TestLGUnroll(unittest.TestCase):
	class TestLGUnroll(unittest.TestCase):
	def test_empty_graph(self):
	lg = LG() # Assuming LG is the class being tested
	result = lg.unroll_to_tpl()
	self.assertEqual(result, expected_output) # Replace expected_output with the actual expected result

[sourcery-ai]

suggestion (testing): Add assertion message for better debugging

Consider adding a custom message to the assertion for better debugging, e.g., self.assertEqual(len(lg._done_dict.keys()), num_keys, f"Failed for {lgn}").

Suggested change

	self.assertEqual(len(lg._done_dict.keys()), num_keys)
	self.assertEqual(len(lg._done_dict.keys()), num_keys, f"Failed for {lgn}")

[myxie]

Hi @awicenec, would you mind reviewing these additions to the PG tests?

[awicenec]

Thanks for going through all of this. Looks good to me and will certainly aid us performing more severe surgeries in the code in the near future.