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

Allow import-tests to account for both the old and new build structure when importing from aria-at #309

Closed
wants to merge 3 commits into from

Conversation

howard-e
Copy link
Contributor

@howard-e howard-e commented Jul 2, 2021

Given the changes introduced by w3c/aria-at#451, the import-tests.js has to account for both the old and new structure when importing tests from https://github.com/w3c/aria-at. Otherwise, rows would be inserted into the TestPlanVersion table without the TestPlanVersion.tests column being populated.

Notes

  1. Assuming your database is fresh, run the following script.
  2. Query the TestPlanVersion table and observe that rows related to both 'versions' of the https://github.com/w3c/aria-at that have been imported have their tests columns populated.
if [[ "$OSTYPE" == "darwin"* ]]; then
    createdb # run this if the PostgreSQL installation is freshly installed
    yarn db-init:dev;
else
    sudo -u postgres yarn db-init:dev;
fi;

yarn sequelize db:migrate;
yarn sequelize db:seed:all;
yarn db-import-tests:dev; # imports most recent commit from aria-at
yarn workspace server db-import-tests:dev -c 4ca7842ea7777b668546e74c9b5ed5b09696d927; # most recent commit hash on aria-at before #451 was merged

…f the aria-at repo since the introduction of the new way of generating the test files
@howard-e howard-e requested review from alflennik and jesdaigle July 2, 2021 13:39
@alflennik
Copy link
Contributor

@howard-e Sorry I somehow missed this PR!

I checked out the branch, and merged develop into it, and then I triggered it by calling http://localhost:3000/api/test/import with a body of { "git_hash": "137716071d58d6d404d3dca45f8d6c12accb8388" }, which is the most recent commit on ARIA-AT.

I got the following error:

Reference file, /Users/alflennik/Projects/aria-at-app/server/scripts/import-tests/tmp/tests/combobox-autocomplete-both/data/references.csv, does not exist!
Error found: Error: ENOENT: no such file or directory, open '/Users/alflennik/Projects/aria-at-app/server/scripts/import-tests/tmp/tests/combobox-autocomplete-both/data/references.csv'
    at Object.openSync (node:fs:505:3)
    at Object.readFileSync (node:fs:401:35)
    at Object.getMostRecentTests (/Users/alflennik/Projects/aria-at-app/server/scripts/import-tests/index.js:131:41)

@howard-e
Copy link
Contributor Author

Thanks for pointing this out @alflennik

combobox-autocomplete-both was an old test plan that has now been removed from https://github.com/w3c/aria-at/tree/master/tests following the merge of w3c/aria-at#417. It seems that the cleanup task when test plans are re-generated with npm run build did not address removing leftover files of this test plan, located at:

because of the presence of these ./build files without the ./tests/combobox-autocomplete-both/ counterpart, this error was thrown.

I will adjust the PR to make an explicit check for the validity of the test plans based on ./tests/.

Additionally, the cleanup task on aria-at when running npm run build will also need to be revised to account for this
cc: @s3ththompson

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 tested very old commits and was able to import them no problem, and I tested the newest commit and was able to import all 16 test plans. I also confirmed I can run the import script from the command line, a cron job and through the HTTP API, and it worked in all three cases 🤩

Code changes look good as well!

@alflennik alflennik force-pushed the fix-import-tests-new-build branch 2 times, most recently from 93a86da to 537dd5d Compare August 19, 2021 15:52
@howard-e howard-e force-pushed the fix-import-tests-new-build branch from 537dd5d to 5d026fe Compare August 19, 2021 15:58
@howard-e
Copy link
Contributor Author

Thanks for the review @alflennik! Closing in favor of an updated server/scripts/import-tests/index.js being introduced in #315.

@howard-e howard-e closed this Aug 31, 2021
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