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(JS tests): fix outdated testdata #722

Merged
merged 1 commit into from
Mar 3, 2023
Merged

Conversation

elsapet
Copy link
Contributor

@elsapet elsapet commented Mar 3, 2023

Description

Branch was out-of-date with main, which meant testdata files were missing secure helmet lines and were failing for reasons unexpected.

Checklist

  • I've added test coverage that shows my fix or feature works as expected.
  • I've updated or added documentation if required.
  • I've included usage information in the description if CLI behavior was updated or added.
  • PR title follows Conventional Commits format

Copy link
Contributor

@didroe didroe left a comment

Choose a reason for hiding this comment

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

Not for this PR but, we really need to scope the integration tests to a single rule again. Adding new rules shouldn't require updating all the other tests.

@cfabianski
Copy link
Collaborator

Not for this PR but, we really need to scope the integration tests to a single rule again. Adding new rules shouldn't require updating all the other tests.

It's annoying I agree

@elsapet
Copy link
Contributor Author

elsapet commented Mar 3, 2023

I agree with the absence rules it is a bit annoying, but there are some cases where it may be useful to run all rules against your test data - it would tell you if you were duplicating an existing rule, for example. Not sure what the best balance is here.

@vjerci
Copy link
Contributor

vjerci commented Mar 3, 2023

Not for this PR but, we really need to scope the integration tests to a single rule again. Adding new rules shouldn't require updating all the other tests.

Imho it's a good thing to have all rules running on tests, otherwise you wouldn't even know you have duplicate rules detection. Also in most cases you'll only need to update 1 or 2 similar rules snapshots.
I think the root of problem here is that we have rules which are very close to being duplicates. Those rules should probably be narrowed in scope of code they detect, removed alltogether, or detect only single from two available or some other solution.. Feels like that's a product problem that we're now trying to solve trough technicalities.

@elsapet elsapet merged commit 3b65905 into main Mar 3, 2023
@elsapet elsapet deleted the fix/fix-outdated-testdata branch March 3, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants