-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add test pass on CI with linting enabled #18709
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
Comments
How slow are the linting checks? How much time would we add to running the pos tests with these additional flags on? The previous issue we had with performance came from executing the pos tests twice. Once with the flags and once without. It took roughly twice the time because we ran the entire compilation pipeline again. Enabling them on the main pos tests should only add an additional fraction of testing time. I would assume that linting is faster than compiling. This is a cost that might be fine for local testing as well.
|
The only problem is the noise in output from warns. However, we could run |
When running |
A lot of pos tests have Werror enabled. We could create a whitelist for those that we want to run with linting, and run the rest in a separate pass (so split pos tests in two). |
Perhaps the number of ugly pos tests that necessarily trip a lint warning is small. They can get
or whatever the option is for turning off lints. (Manual bookkeeping is a dreadful maintenance burden; I think vulpix's programmatic model was weak on that score.) |
Resolves #18709 Just a random sample of ~1000 pos tests that do not fail with linting flags enabled Benefits: - Limited possible impact on the performance of tests - New tests won't be run with linting, so they won't fail even if fatal warnings are enabled.
To prevent compiler crashes from happening, we should test all compiler flags on a complex and large set of code snippets. A good candidate for that is the
pos
test suite. However, to not make the local tests any longer, they should run only on CI.The text was updated successfully, but these errors were encountered: