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.

Sign up for GitHub

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

Jump to bottom

Add lint flags to a random sample of pos tests #18728

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Add lint flags to a random sample of pos tests #18728

merged 2 commits into from
Oct 20, 2023

Conversation

szymon-rd
Copy link
Contributor

@szymon-rd szymon-rd commented Oct 19, 2023 •
edited
Loading

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.

@szymon-rd szymon-rd changed the title Add lint flags to random sample of pos tests Add lint flags to a random sample of pos tests Oct 19, 2023
@nicolasstucki nicolasstucki self-requested a review October 19, 2023 17:05
@nicolasstucki nicolasstucki self-assigned this Oct 19, 2023
nicolasstucki
nicolasstucki approved these changes Oct 20, 2023
View reviewed changes
compiler/test/dotc/pos-linting.allowlist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: maybe sort this file

@som-snytt
Copy link
Contributor

som-snytt commented Oct 20, 2023 •
edited
Loading

It can be useful just to know it doesn't crash or cause unexpected warnings.

Edit: speculation follows

An idea just popped into my head, not necessarily practical, is that all tests should have the same linted state before and after a PR.

It's possible that e.g. -Xlint:infer-any (speculatively) would start to warn after a change because of typechecking or codegen. You'd want to know about the difference. The warning is not necessarily a bug.

Also, any change might indicate UX change (maybe a progression).

By "speculatively", I mean "speculoosly".

@szymon-rd
Copy link
Contributor Author

Snapshot tests would help as well, that is true. Currently, these tests are mainly meant to prevent crashes, and the feature is tested in a separate set of tests.

@szymon-rd szymon-rd merged commit 89a8fce into main Oct 20, 2023
@szymon-rd szymon-rd deleted the test-linting branch October 20, 2023 15:08
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
@WojciechMazur WojciechMazur mentioned this pull request Jun 22, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jun 23, 2024
@WojciechMazur
Backport "Add lint flags to a random sample of pos tests" to LTS (#20732
d7ba229 
)

Backports #18728 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolasstucki nicolasstucki nicolasstucki approved these changes

Assignees

@szymon-rd szymon-rd

Labels
None yet
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.

Add test pass on CI with linting enabled
4 participants
@szymon-rd @som-snytt @nicolasstucki @Kordyjan