-
Notifications
You must be signed in to change notification settings - Fork 537
test: skip test_language_package
in long tests
#4327
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
Conversation
test_language_package
is not skipped if LONG_TEST is disabled.[Issue #4322]test_language_package
is skipped if LONG_TEST is disabled.[Issue #4322]
test_language_package
is skipped if LONG_TEST is disabled.[Issue #4322]test_language_package
in long tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I edited your PR title to fix the gitlint error (it was just a bit long) and there's an isort error below you'll need to fix before we can merge. But this looks good other than the linter issues! I initially thought "oh, it seems like such a pain to set each skip individually" but since I think we'll be slowly adding these back as we tune them having it done this way will make that a lot easier, so thank you for that.
Oh, and if you need more info on the linters, that's here in the contributor docs: https://cve-bin-tool.readthedocs.io/en/latest/CONTRIBUTING.html#running-linters |
Ugh, looks like merging the new language tests broke this and I wasn't able to fix it during the merge conflict resolution. I'm going to flag it for me to look at it tomorrow, since I'm unlikely to get to it today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like when I did the merge i accidentally deleted the parser lines added in by the other PR, so as a result the test broke. I'm doing some testing on my local machine to make sure I'm right about the fix, but in the meantime if you want to fix your branch I've got one suggestion below and you can probably figure out what the others need to look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so it's definitely a problem where the merge missed adding the parser name as the second argument. (You can see it as deleted lines in the diff right now) Fixing that second argument allows the new part of the test to run and the disabling still works as expected.
It doesn't look like I can fix this PR for you from the web interface, so I'd very much appreciate it if you could add those in! Note that because of the merge your local branch is probably showing as out of date with the github branch this PR is based on so you've got a few options:
git pull
from your remote branch into your local branch to get them back into sync then push a fix that adds the parsers in as second argument.- make a new branch / new PR and skip dealing with getting them in sync
- fix your local branch and
git push -f
to this one to over-write the merge that didn't work perfectly
Probably the 1st is the easiest option?
If you can't get to it over the next couple of days or syncing git branches is too much darned work and you'd like me to handle it for you, let me know and I'll see what I can do.
What is up with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the triage.json is related to another PR -- github is showing the diff probably because of the way things were merged. It makes code review a bit more annoying but shouldn't impact getting this merged.
Thank you for iterating on this with me! Looks like the tests are all behaving as expected now, so let's get this merged!
Description
closes #4322
Checklist