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

Update hashTests function to support the migration of test results across test plan versions using the v2 test format #880

Merged
merged 12 commits into from
Jan 29, 2024

Conversation

howard-e
Copy link
Contributor

@howard-e howard-e commented Dec 18, 2023

This PR updates aria.js to account for properties to be ignored during a v2 test format version update so applicable results can be transferred over during phase update process.

This should address the scenario described in #877 and produce the expected results along with the additional requirements:

The procedure that manifest this bug was:

  1. Command button V23.12.06 was in draft review and had three test runs in the test queue.
  2. Results were present for all the runs.
  3. We discovered that a change was needed to the settings specified for two VoiceOver commands.
  4. We made V23.12.13 of the plan with the changes to the VoiceOver commands and merged it to master so it showed on the data management page as R&D Complete.
  5. We advanced V23.12.13 of the command button test plan to draft review.

Expected results:

  1. Since V23.12.06 is deprecated, its runs no longer show in the test queue.
  2. To avoid unnecessary loss of manual test work, equivalent runs of V23.12.13 are added to the test queue, preserving as many test results as possible.

Actual results:

  1. Runs of V23.12.06, which became deprecated, remain in the test queue.
  2. Runs of V23.12.13 that we subsequently added to the test queue were empty, so testers have to start over even for tests that were not changed.

@howard-e howard-e marked this pull request as ready for review December 20, 2023 16:20
@howard-e howard-e changed the title Update hashTests function to account for v2 test format version Update hashTests function support the migration of test results across test plan versions using the v2 test format Dec 20, 2023
Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

I am approving! There's a couple of minor comments but it all feels optional. I verified the tests are passing (big thank you for buffing up the test coverage), and also I rebuilt the app from scratch to verify that it was still able to compare test versions without introducing duplicates.

server/util/aria.test.js Outdated Show resolved Hide resolved
server/util/aria.js Outdated Show resolved Hide resolved
server/util/aria.test.js Show resolved Hide resolved
…st format version update so applicable results can be transferred over during phase update
* Rename testWithNoIds -> testWithOmittedAttributes
…ionally, now deleting found duplicates as there is an error when tested against prod data where unnecessary duplicates were created
);
} else {
// Remove any accidentally created TestPlanVersion rows created prior to migration
await queryInterface.sequelize.query(
Copy link
Contributor Author

@howard-e howard-e Jan 22, 2024

Choose a reason for hiding this comment

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

For reviewers, found an issue during checks against production where several TestPlanVersions were created as duplicates (yet had non-matching hashedTests, because of some failed sync between pushes I suppose).

Also broke this out into a separate utility because several migrations may have to make use this in the future as well. Immediately seeing a need for #863 to use this as well.

Included 6f8dfb9 in the recent staging push, which was required. It displays why this change is needed.

@howard-e howard-e force-pushed the fix-v2-results-update branch from e832a86 to 3c6d020 Compare January 22, 2024 18:18
howard-e added a commit that referenced this pull request Jan 22, 2024
…test

format version to also include (an empty) assertionExceptions so hashes will remain in sync; include utility introduced in #880
@howard-e howard-e changed the title Update hashTests function support the migration of test results across test plan versions using the v2 test format Update hashTests function to support the migration of test results across test plan versions using the v2 test format Jan 23, 2024
Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and accompanying expanded test coverage. I was able to confirm that this was functioning as expected by rebuilding the DB and running the unit tests. The new code also has greater clarity than what it replaces. Approving!

Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

I'm happy to approve a second time on this one. I like the idea of making it easy to recalculate the hashes, it does seem pretty likely it might become necessary down the line. I took a look at the broader changes to the tests as well and didn't see anything that rang any alarm bells. I see the tests are passing. Nice work!

@alflennik alflennik merged commit eaf9109 into main Jan 29, 2024
2 checks passed
@alflennik alflennik deleted the fix-v2-results-update branch January 29, 2024 21:42
howard-e added a commit that referenced this pull request Feb 6, 2024
* Update client/resources

* Update graphql-schema.js and testsResolver

* Update assertionResultsResolver.js

* Update import-tests script to include assertion.assertionExceptions (0 level assertions will be considered internally as 'EXCLUDE')

* Update getMetrics and queries to be aware of 'excluded' assertions for commands

* Update import branch for testing

* Update import branch for testing

* Remove 'exclude' property check reliance on frontend

* Revert graphql fragment usage

* Fix missing renderableContent in anon viewer query

* Rename instances of Command.settings to Command.atOperatingMode in graphql-schema.js

* Include migration to update existing test plan versions using the v2 test
format version to also include (an empty) assertionExceptions so hashes will remain in sync; include utility introduced in #880

* Move atOperatingMode setting to TestsService, getTests

* Missing comma

* Update comments with reasoning for filtering out assertions that can possibly have no ids

* Update convertAssertionPriority usage

* Revert test branch being used back to master

* Break out in-line filter into function sufficiently support readability purposes

---------

Co-authored-by: Stalgia Grigg <stalgia@bocoup.com>
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