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(tester): only regenerate expected test outputs when necessary #699

Merged
merged 2 commits into from
Mar 7, 2025

Conversation

niklasdewally
Copy link
Collaborator

@niklasdewally niklasdewally commented Mar 6, 2025

For test stages that use a model JSON, only regenerate the expected test outputs if the test stage fails.

BACKGROUND

In 8636432 (feat!: add parent symbol tables (#680), 2025-02-17), we added an id field to symbol-tables, allowing the reconstruction of pointers during de-serialisation.

These ids are set by a global variable; therefore, they tend to change from run to run of the tester. However, they are ignored in the implementation of Eq for Model/SubModel, so this does not affect test correctness.

After this change, when running the test suite with ACCEPT=true, new expected output files would be created for all test models containing different ids.

This caused our git diffs to always contain changes to the ids in every single test case, cluttering them up.

As id changes do not cause tests to fail, this commit means that an id change will not cause a new expected test output file to be generated.

@niklasdewally niklasdewally force-pushed the nik/fix-tester-id-churn branch from f1221aa to e33c076 Compare March 6, 2025 17:41
@niklasdewally niklasdewally self-assigned this Mar 6, 2025
For test stages that use a model JSON, only regenerate the expected test
outputs if the test stage fails.

BACKGROUND

In 8636432 (feat!: add parent symbol tables (#680), 2025-02-17), we
added an id field to symbol-tables, allowing the reconstruction of
pointers during de-serialisation.

These ids are set by a global variable; therefore, they tend to change
from run to run of the tester. However, they are ignored in the
implementation of `Eq` for Model/SubModel, so this does not affect test
correctness.

After this change, when running the test suite with ACCEPT=true, new
expected output files would be created for all test models containing
different ids.

This caused our git diffs to always contain changes to the ids in every
single test case, cluttering them up.

As id changes do not tests to fail, this commit means that an id change
will not cause a new expected test output file to be generated.
@niklasdewally niklasdewally force-pushed the nik/fix-tester-id-churn branch from e33c076 to 6f7c7e3 Compare March 6, 2025 17:42
Copy link
Contributor

github-actions bot commented Mar 7, 2025

Code and Documentation Coverage Report

Documentation Coverage

This PR:

conjure_core: 3% with examples, 52% documented -- 8/143/275
conjure_oxide: 0% with examples, 16% documented -- 0/6/35
minion_rs: 30% with examples, 82% documented -- 2/23/112
tree-morph: 50% with examples, 100% documented -- 9/22/22

Main:

conjure_core: 3% with examples, 52% documented -- 8/143/275
conjure_oxide: 0% with examples, 16% documented -- 0/6/35
minion_rs: 30% with examples, 82% documented -- 2/23/112
tree-morph: 50% with examples, 100% documented -- 9/22/22

View full documentation coverage for main, this PR

Code Coverage Summary

This PR: Detailed Report

  functions..: 66.8% (740 of 1108 functions)
  branches...: no data found

Main: Detailed Report

  functions..: 66.8% (740 of 1108 functions)
  branches...: no data found

Coverage Main & PR Coverage Change

Functions coverage changed by 0.00% and covered lines changed by 0
Branches... coverage: No comparison data available

@ozgurakgun
Copy link
Contributor

brilliant, thanks! why the new integration test as well? are you testing something related to this PR specifically?

@niklasdewally
Copy link
Collaborator Author

brilliant, thanks! why the new integration test as well? are you testing something related to this PR specifically?

It was to test this PR: I changed the test and ran ACCEPT=true, and this was the resulting git diff.

@ozgurakgun ozgurakgun merged commit be0fc58 into main Mar 7, 2025
14 checks passed
@ozgurakgun ozgurakgun deleted the nik/fix-tester-id-churn branch March 7, 2025 15:56
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.

2 participants