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

All UI tests fail (OSOE-748) #327

Closed
sarahelsaig opened this issue Dec 3, 2023 · 13 comments
Closed

All UI tests fail (OSOE-748) #327

sarahelsaig opened this issue Dec 3, 2023 · 13 comments
Assignees
Labels
bug Something isn't working

Comments

@sarahelsaig
Copy link
Member

sarahelsaig commented Dec 3, 2023

It started to happen some time earlier today and retroactively applies to any new UI test executions in CI. Some examples:

Even NugetTest is affected. For this reason I think the problem is not with our code. The error message talks about chalk in html-validate:

An assertion during the page change event has failed on page https://localhost:9096/ (Test Site - Blog).
The test failed with the following exception: Atata.Cli.CliCommandException: /usr/local/lib/node_modules/html-validate/node_modules/@sidvind/better-ajv-errors/lib/cjs/index.js:933
var import_chalk = __toESM(require("chalk"));
                           ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /usr/local/lib/node_modules/html-validate/node_modules/@sidvind/better-ajv-errors/node_modules/chalk/source/index.js from /usr/local/lib/node_modules/html-validate/node_modules/@sidvind/better-ajv-errors/lib/cjs/index.js not supported.
Instead change the require of /usr/local/lib/node_modules/html-validate/node_modules/@sidvind/better-ajv-errors/node_modules/chalk/source/index.js in /usr/local/lib/node_modules/html-validate/node_modules/@sidvind/better-ajv-errors/lib/cjs/index.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/usr/local/lib/node_modules/html-validate/node_modules/@sidvind/better-ajv-errors/lib/cjs/index.js:933:28)
    at Object.<anonymous> (/usr/local/lib/node_modules/html-validate/dist/cjs/core.js:7:23)
    at Object.<anonymous> (/usr/local/lib/node_modules/html-validate/dist/cjs/html-validate.js:7:12)
    at Object.<anonymous> (/usr/local/lib/node_modules/html-validate/bin/html-validate.js:4:1) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v18.18.2.

CLI command: bash -login -c "html-validate \"51fb2306-e5db-43e3-8e6c-496a40b8e80a.html\" -f stylish"
Working directory: /home/runner/work/Open-Source-Orchard-Core-Extensions/Open-Source-Orchard-Core-Extensions/NuGetTest/test/Lombiq.OSOCE.NuGet.Tests.UI/bin/Debug/net6.0/HtmlValidationTemp
Exit code: 1
   at Atata.Cli.ProgramCli.ValidateResult(CliCommandResult result)
   at Atata.Cli.ProgramCli.Execute(String arguments)
   at Atata.Cli.HtmlValidate.HtmlValidateCli.Validate(String path, HtmlValidateOptions options)
   at Atata.HtmlValidation.HtmlValidator.<>c__DisplayClass18_0.<ExecuteCliCommand>b__0()
   at Atata.HtmlValidation.HtmlValidator.ExecuteFunction[TResult](String sectionMessage, Func`1 function)
   at Atata.HtmlValidation.HtmlValidator.ExecuteCliCommand(String workingDirectory, String htmlFileName, String formatter)
   at Atata.HtmlValidation.HtmlValidator.Validate(String html)
   at Lombiq.Tests.UI.Extensions.HtmlValidationUITestContextExtensions.ValidateHtml(UITestContext context, Action`1 htmlValidationOptionsAdjuster)
   at Lombiq.Tests.UI.Extensions.HtmlValidationUITestContextExtensions.AssertHtmlValidityAsync(UITestContext context, Action`1 htmlValidationOptionsAdjuster, Func`2 assertHtmlValidationResultAsync)
   at Lombiq.Tests.UI.Extensions.HtmlValidationOrchardCoreUITestExecutorConfigurationExtensions.<>c__DisplayClass0_0.<<SetUpHtmlValidationAssertionOnPageChange>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.Collections.Generic.EnumerableExtensions.AwaitEachAsync[TItem](IEnumerable`1 source, Func`2 asyncOperation)
   at Lombiq.Tests.UI.Services.UITestContext.TriggerAfterPageChangeEventAsync()

Jira issue

@sarahelsaig sarahelsaig added the bug Something isn't working label Dec 3, 2023
@github-actions github-actions bot changed the title All UI tests fail All UI tests fail (OSOE-748) Dec 3, 2023
@Piedone
Copy link
Member

Piedone commented Dec 4, 2023

Would you be able to work on this ASAP?

This looks like something https://github.com/atata-framework/atata-htmlvalidation can fix, and is most possibly an underlying issue with the html-validate NPM package. My hunch is some auto-update of an NPM package happening behind the scenes due to an open version selector.

A temporary hotfix to be merged ASAP can be to disable HTML validation in our tests. Then we can check if downgrading html-validate a bit via HtmlValidatePackageVersion here resolves the issue for a still temporary fix but after which we can restart HTML validation. And eventually, this needs to be fixed in the underlying libraries.

@sarahelsaig
Copy link
Member Author

The problem must be CI-specific, because I can't repro it locally, and we already use Atata.HtmlValudation which has a pinned package version that hasn't been changed since September. Additionally, specifying the current latest html-validate version doesn't do anything either.

@Piedone
Copy link
Member

Piedone commented Dec 4, 2023

@sidvind/better-ajv-errors had an update yesterday, most possibly this broke things, since html-validate has an open version selector for it (the version has a lot more changes than indicated on the linked version page).

Most possibly this happens anywhere, you just need to clear the NPM cache.

@YevgeniyShunevych
Copy link
Contributor

Let me share my findings. Agree with you, @Piedone. Seems like @sidvind/better-ajv-errors introduced this error. I was getting the same error through whole day today locally and on CI. The same error was raising for all version of html-validate above 7.0.0. And just right now after another reinstall of html-validate package locally the error went away. On CI it is also fine now. You could verify on your side. Trying to figure out how it was fixed. Weird, there were no package releases of html-validate and @sidvind/better-ajv-errors.

@Piedone
Copy link
Member

Piedone commented Dec 4, 2023

Thank you for chiming in, Yevgeniy!

BTW that now we have NPM caching enabled for GHA builds on BuildJet, might affect this.

@sarahelsaig
Copy link
Member Author

This got fixed on its own with the new @sidvind/better-ajv-errors package release about an hour before your last message. The only change in this version is Revert dependency chalk to v4.

I'm not sure we can do anything about this? We could maintain an NPM lock file and copy it into the working directory before HtmlValidator.Validate(html) is executed, but that doesn't feel very practical.

@Piedone
Copy link
Member

Piedone commented Dec 4, 2023

Great then! This looks like it needs to be fixed in html-validate by pinning all dependency versions; please open an issue there.

@sarahelsaig
Copy link
Member Author

@Piedone
Copy link
Member

Piedone commented Dec 5, 2023

Thanks. Also, wouldn't perhaps Atata.HtmlValidation be able to pin versions too with a package-lock @YevgeniyShunevych? Because otherwise there's still a chance that anything in the NPM dependency chain will pull a similar breaking change.

@YevgeniyShunevych
Copy link
Contributor

@Piedone, Atata.HtmlValidation is pinned to a specific version of html-validate package by default, which is currently 8.3.0. From time to time I increase that version during releases. Under the hood, if installed version doesn't match, npm install -g html-validate@8.3.0 is executed.

Can we pin package-lock in our case? I am not very familiar with npm.

@Piedone
Copy link
Member

Piedone commented Dec 7, 2023

Hmm, oh yeah, there's the custom html-validate version selection option too and the whole dynamic NPM installation that complicates things. This all comes down to how RequireVersion() can work.

Perhaps it would be possible to add a package-loc and package.json file for the default html-validate version and make RequireVersion() use that if the version matches? Because now it installs the package like you add any new package the first time. But with these two files, you can start from a state where the package was already added, you just install it.

Admittedly, this would only really work for the current default html-validate version and you'd need to keep the two JSON files up-to-date too. However, it would also make sure that the Atata library indeed works, also in the future, with the given default html-validate version , regardless of how the underlying dependencies may have new versions.

@sarahelsaig
Copy link
Member Author

I agree, you'd only apply the package lock if the version selector is the default. Hopefully if the user went out of their way to changed it, they know what they are doing.
I think could be as simple as having a package-lock.json file as embedded resource and copying it into the current directory before the npm install -g html-validate@8.3.0 is called, only if one doesn't exist already and the version selector is in the default value.

@sarahelsaig
Copy link
Member Author

My html-validate PR has been resolved so I think this issue can be closed now. I've opened a followup issue regarding the lock file discussion in atata-framework/atata-htmlvalidation: atata-framework/atata-htmlvalidation#7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants