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

Move EV-specific tests to cabf_ev #445

Merged
merged 1 commit into from
Jun 5, 2020
Merged

Move EV-specific tests to cabf_ev #445

merged 1 commit into from
Jun 5, 2020

Conversation

sleevi
Copy link
Contributor

@sleevi sleevi commented Jun 4, 2020

This moves the lints for the EV Guidelines into cabf_ev, ensuring a consistent sourcing and reference.

Closes #439

@sleevi sleevi requested a review from cpu June 4, 2020 01:34
@sleevi
Copy link
Contributor Author

sleevi commented Jun 4, 2020

@cpu My first request here, so let me know if I've botched anything here.

I realize this does "two things, not one" - in that in the process of moving, it aligns everything to use "EVGs" to match the "BRs" reference of the existing lints. Happy to pull that into a separate PR, if useful, or to update the description to better reflect that.

Copy link
Contributor

@cardonator cardonator left a comment

Choose a reason for hiding this comment

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

Personally, I think this is quite grokable and there is no need for a second PR. 👍

It does make me wonder why we have the Baseline Requirements and the EV Guidelines. Are the EVGs the Pirate's code? 😄

@cpu
Copy link
Member

cpu commented Jun 4, 2020

I realize this does "two things, not one" - in that in the process of moving, it aligns everything to use "EVGs" to match the "BRs" reference of the existing lints. Happy to pull that into a separate PR, if useful, or to update the description to better reflect that.

No worries, I don't think it makes this hard to review to have both things done at once 👍 Thanks for the PR!

cpu
cpu previously requested changes Jun 4, 2020
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

The diff here looks good to me. Thanks @sleevi!

One strange thing is the integration test failure:

TestCorpus: corpus_test.go:163: expected lint "e_ev_valid_time_too_long" to have result fatals: 0    errs: 0    warns: 0    infos: 0    got fatals: 0    errs: 5    warns: 0    infos: 0   

It looks like prev. integration tests weren't expecting any findings for this lint and now there are 5 error results. I can't immediately spot a reason for this but suspect it's something silly. Can you update the integration/config.json so this will pass (and maybe see if the new 5 err results make sense?).

@sleevi
Copy link
Contributor Author

sleevi commented Jun 4, 2020

Yup, that’s what lead to #446 , as this is odd and I do want to dive in before just tweaking tests blindly :)

@sleevi
Copy link
Contributor Author

sleevi commented Jun 5, 2020

@cpu The failure was due to #447 , so I've rebased and updated that PR (still used to Chromium's rebase-based workflow, versus GitHub's seeming preference for a merge workflow, so apologies if I've stuffed up)

The CI should be nice and happy ("it works on my machine")

@zakird zakird dismissed cpu’s stale review June 5, 2020 01:46

Problem has been resolved, tests are now passing.

@zakird zakird merged commit ecf8678 into zmap:master Jun 5, 2020
@cpu
Copy link
Member

cpu commented Jun 5, 2020

Thanks!

@sleevi sleevi deleted the ev_lints branch June 7, 2020 16:49
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.

Move EV-related lints into cabf_ev
4 participants