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(node): Resolve ace_ibma.Checker is not a constructor error #1831

Closed

Conversation

Rahul-Sivananda
Copy link
Contributor

@Rahul-Sivananda Rahul-Sivananda commented Feb 7, 2024

  • Try to resolve the ace_ibma.Checker is not a constructor error

This PR is related to the following issue(s):

Additional information can be found here:

Currently, the generated ace-node.js file is updated multiple times when running in multiple threads, causing the ace_ibma.Checker is not a constructor error while reading it when the file is being overwritten.

This commit does the following things:

  1. Read the generated ace-node.js file if it exists and create the accessibility checker without overwriting the file.
  2. Throw error if there was a problem in writing the ace-node.js file instead of attempting to create the constructor from the non-existing file.

Due to the above change, we must update the generated file name.
The current file is stored as <cache-folder-name>/ace-node.js. If the cache folder is not cleared, this would cause issues when either rule archive or package version is changed, as we are no longer overwriting the file.

Suffixing rule and package version to the file name (e.g. ace-node-3.0.0-3.1.67.js) prevents this from happening.

Testing reference:

To test, run multiple accessibility tests using jest parallelly, ace_ibma.Checker is not a constructor error should not occur.
To verify that the generated file is not being overwritten, run fswatch -uvx <cache-folder-path>

I have conducted the following for this PR:

  • I validated this code in Chrome and FF
  • I validated this fix in my local env
  • I provided details for testing
  • This PR has been reviewed and is ready for test
  • I understand that the title of this PR will be used for the next release notes.

Currently, the generated `ace-node.js` file is updated multiple times in
threads. This commit reads the `ace-node.js` file if it exists and
will create the accessibility checker without overwriting the file.

Another change is to throw error if there was a problem in writing the
`ace-node.js` file instead of attempting to create the constructor from
the non existing file.
The current generated file name is stored as <cache-folder>/ace-node.js
If the cache folder is not cleared, this would cause problems when
either rule archive or package version is changed as we are no longer
overwriting the file

Suffixing rule and package version to the file name prevent this from
happening.
tombrunet
tombrunet previously approved these changes Feb 12, 2024
@philljenkins philljenkins changed the title fix(node): resolve ace_ibma.Checker is not a constructor error fix(node): Resolve ace_ibma.Checker is not a constructor error Feb 15, 2024
@Rahul-Sivananda
Copy link
Contributor Author

Rahul-Sivananda commented Feb 16, 2024

I am struggling to understand the pipeline failure reasons, and I'm unable to reproduce them locally.

For example, results for npm run wdio locally :

result of 'npm run wdio' command, all tests green

And npx mocha -R dot test/mocha/aChecker.Fast/aChecker.Baselines/*.test.js
Result of 'npx mocha -R dot test/mocha/aChecker.Fast/aChecker.Baselines/*.test.js' command, all tests green

Do I need to update something else for the change to work?

the github actions set ruleArchiveVersion as <pr_number>/merge, this is
obtained through GITHUB_REF environment variable, and this problems in the github actions.
The version should be a valid file name that can be used in both unix
and non unix filesystems.

This PR sets the commit sha obtained from GITHUB_SHA environment
variable as the ruleArchiveVersion
@Rahul-Sivananda
Copy link
Contributor Author

Rahul-Sivananda commented Feb 16, 2024

I'm testing out setting the commit sha as the archive version instead of the GITHUB_REF (<pr_number>/merge). This might cause problems in packaging.

I replaced let releaseTag = (process.env.GITHUB_REF || "").substring(10); with let releaseTag = (process.env.GITHUB_REF || "").substring(10).replace("/merge", ""); this seems to be more failsafe.

@tombrunet
Copy link
Member

Moving to #1845

@tombrunet tombrunet closed this Feb 16, 2024
@Rahul-Sivananda Rahul-Sivananda deleted the fix-reuse-ace-node-file branch February 16, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants