Generated tests and review pages output improvements

[howard-e]

Preview Tests - FORKED REPO PREVIEW

This addresses a few issues identified with the automation/workflow scripts flow. See #384.

Test Plans generation script noisy output

When running npm run create-all-tests to generate test plans, many of the messages were unnecessary when one of the main goals was only understanding if anything unexpected had happened.

Verbose Output

The decision taken here was to completely silence the output except for displaying errors and providing a summary of how many tests have been generated.
A note on how many log lines have been suppressed along with instructions on how to view those messages is provided which means the original verbose output is now behind a --verbose flag.

Non-Verbose Output

Conflicts created by generated files from PR action + local development changes

• The generated output for the npm run create-all-tests and npm run review-tests will continue to work as normal. The only difference is that the in-line files they generated are now gitignored stored in a build folder. This means means there will no longer be a chance for merge conflicts to occur on those in-line generated files when having to push/pull to/from the repository.
• Those scripts however, now support a --build flag to create a dedicated build artifact folder which will have those generated files. These files are updated via a GitHub Action with npm run build which will then be referenced in the PR's Preview Tests link instead.
• This may also require persons pulling in the latest commit from the base branch (which the PR will also notify you of) before merging to avoid any possible merge conflicts but overall, the merge conflicts situation should be somewhat mitigated for the time being.
• NOTE: This is temporary until the Netlify deployments are in place so that we may do away with having generated files (build folder) in the repository altogether
• NOTE2: Would recommend filtering out *.css, *.html and *.json files for the review to make it easier to navigate. Those are generated files which have been moved.

TODOS:

• Remove in-line file generation
  • Update documentation on usage of build folder for local development
• Provide a npm run validate command that will output the validity status of the written test plans
  • Update documentation on usage
• Be able to pass a specific test plan directory to npm run build
  • Update documentation on usage
• Be able to pass a specific test plan directory to npm run validate
  • Update documentation on usage
• Provide a npm run cleanup command that will remove the build directory

[jscholes]

The generated output for the npm run create-all-tests and npm run review-tests will continue to work as normal.
...
Those scripts however, now support a --build flag to create a dedicated build artifact folder which will have those generated files.

I don't know how to reconcile these two points. Are you saying that the scripts will continue to create output artifacts in the tests directory, as well as the dedicated build dir if --build is used? If so, what is the point of the former?

[jscholes]

Takeaways from today's app dev meeting:

1. Discard current inline generated assets approach; having a single build dir is cleaner and still allows local preview of that directory.
2. Have separate build and validate commands:
   • build does the job that create-all-tests and review-tests currently do, targeting that single build dir.
   • validate allows local testing of test plans for errors, without generating any built assets.

Longer term goals:

1. Have a cleanup/clean command as a convenience method of deleting the build directory.
2. Have a serve command or similar to spin up a web server pointing at a temporary copy of the build dir, which is automatically deleted when the command is stopped. Bonus points if it does live refresh as files are edited.
3. Add the ability to target a specific test plan directory in the validate and build commands, to avoid unnecessary regeneration, SSD writes, etc.
4. Add a "smarter" diff-based system to build/validation scripts which automatically only targets plans that have been changed since the last build. E.g. if there are 40 plans and only 1 has been modified, indiscriminately regenerating the other 39 is wasteful and performance will degrade as the number of plans grows.

[howard-e]

2. Have a serve command or similar to spin up a web server pointing at a temporary copy of the build dir, which is automatically deleted when the command is stopped. Bonus points if it does live refresh as files are edited.
3. Add a "smarter" diff-based system to build/validation scripts which automatically only targets plans that have been changed since the last build. E.g. if there are 40 plans and only 1 has been modified, indiscriminately regenerating the other 39 is wasteful and performance will degrade as the number of plans grows.

These 2 can be addressed in separate PRs.

[alflennik]

Looks great! I'm going to approve!

Although I did run all the commands, I mostly reviewed the code itself, so I do think James or someone on the test author side might be interested in reviewing from a user's perspective.

[jscholes]

@howard-e I know this has been merged, but I have some questions. Apologies I'm not leaving them directly on the diff, which is unusable with a screen reader because of the number of files modified.

In package.json

    "build": "npm run create-all-tests -- --testplan=$npm_config_testplan; npm run review-tests  -- --testplan=$npm_config_testplan",

What is $npm_config_testplan and how is it set?

    "build": "npm run create-all-tests -- --testplan=$npm_config_testplan; npm run review-tests  -- --testplan=$npm_config_testplan",
    "validate": "npm run create-all-tests -- --validate --testplan=$npm_config_testplan",

Nitpicking, but it's a little odd to have a script called create-all-tests that accepts both:

• an argument to not create all tests (--testplan), and
• an argument to not created anything (--validate).

    "cleanup": "rm -fr ./build",

Noting that this is specific to systems with an rm command, so not Windows (the primary OS at PAC for what that's worth). Also that it seemingly can be executed from anywhere within the repo, but only actually works at the root.

Other stuff

When I run npm run build or npm run validate from the root of the repo, I get:

ERROR: Unable to find valid test plan(s).

Is this related to the NPM config thing I asked about above? Are the changes documented somewhere?

Thanks for your work on this!

[jscholes]

Also, just to note, @alflennik wrote:

I do think James or someone on the test author side might be interested in reviewing from a user's perspective.

I would've been happy to do this before it was merged, had my review been requested or if I'd actually been mentioned. So apologies again that these comments are coming in after the fact.

[howard-e]

Noted. That point had slipped us as well when merging so our apologies there. It's completely reasonable to revert for now if you'd rather to do a more in depth review.

[jscholes]

It's completely reasonable to revert for now if you'd rather to do a more in depth review.

I appreciate that. I'd like to understand the appropriate usage pattern of the scripts / if I'm doing anything wrong, then take a look at the output to assess whether it resolves some of the accessibility concerns. I'd also like to see OS-specific concerns (like the use of rm) be fixed up if possible.

If your timeline allows us to revert, review, fix up and merge in again over the next 1-1.5 weeks, I think we should. I'm certainly happy to review this week. Otherwise, if reverting would likely introduce greater delays than 1.5 weeks, we should just move forward and file any issues as follow-ups.

[howard-e]

    "build": "npm run create-all-tests -- --testplan=$npm_config_testplan; npm run review-tests  -- --testplan=$npm_config_testplan",

What is $npm_config_testplan and how is it set?

It's an environment variable injected by node once that argument is passed to a node script. In this particular case, it's set by providing an argument to the build/validate command as such: npm run build --testplan=checkbox. When you run npm run build, it is done on all existing test plans by default.

    "build": "npm run create-all-tests -- --testplan=$npm_config_testplan; npm run review-tests  -- --testplan=$npm_config_testplan",
    "validate": "npm run create-all-tests -- --validate --testplan=$npm_config_testplan",

Nitpicking, but it's a little odd to have a script called create-all-tests that accepts both:

• an argument to not create all tests (--testplan), and
• an argument to not created anything (--validate).

You would rather separate scripts that that are dedicated to either functions rather than this argument based approach on the original? Or do you think a rename of the original script would be appropriate?

    "cleanup": "rm -fr ./build",

Noting that this is specific to systems with an rm command, so not Windows (the primary OS at PAC for what that's worth). Also that it seemingly can be executed from anywhere within the repo, but only actually works at the root.

You're quite correct. A patch should follow for the full Windows support there.

Other stuff

When I run npm build or npm validate from the root of the repo, I get:

ERROR: Unable to find valid test plan(s).

Is this related to the NPM config thing I asked about above? Are the changes documented somewhere?

The script changes and additions are documented within a newly created file, docs/LOCAL_DEVELOPMENT.md. The README.md has also been updated to reflect as such about that documentation. If you get the chance to review, please let me know if the contents of that document are sufficient.

This specific error however may be related to it being ran under a Windows environment.

[howard-e]

It's completely reasonable to revert for now if you'd rather to do a more in depth review.

I appreciate that. I'd like to understand the appropriate usage pattern of the scripts / if I'm doing anything wrong, then take a look at the output to assess whether it resolves some of the accessibility concerns. I'd also like to see OS-specific concerns (like the use of rm) be fixed up if possible.

Yes, I believe this would be best as you weren't allowed ample time to review and to also include that platform-agnostic patch.

If your timeline allows us to revert, review, fix up and merge in again over the next 1-1.5 weeks, I think we should. I'm certainly happy to review this week. Otherwise, if reverting would likely introduce greater delays than 1.5 weeks, we should just move forward and file any issues as follow-ups.

I don't believe this is a problem at the moment so I'll go ahead with reverting for now.

[jscholes]

@howard-e Thanks for the clarification. Getting it running on Windows is critical, to allow review and ultimately unblock some of our workflows. So I think we should proceed with a new PR, and we can continue discussion there. What is the expected timeline for that new PR to go up?