Skip to content

Display out of sync drifts only in deep mode#1235

Merged
sundowndev merged 7 commits intosnyk:mainfrom
smaftoul:fix/ouf-of-sync-only-deep-mode
Nov 26, 2021
Merged

Display out of sync drifts only in deep mode#1235
sundowndev merged 7 commits intosnyk:mainfrom
smaftoul:fix/ouf-of-sync-only-deep-mode

Conversation

@smaftoul
Copy link
Copy Markdown
Contributor

@smaftoul smaftoul commented Nov 25, 2021

Q A
🐛 Bug fix? yes
🚀 New feature? no
⚠ Deprecations? no
❌ BC Break no
🔗 Related issues #964
❓ Documentation no

Description

A fix for #964.

It breaks some tests because I think the fake analysis in tests aren't created with analyzerOptions which in some tests should contain Deep: true. Other tests should probably fix the output.

Can you tell me if this approach to fix tests seems right ?

@smaftoul smaftoul force-pushed the fix/ouf-of-sync-only-deep-mode branch 2 times, most recently from 4f26e93 to 9aa787f Compare November 25, 2021 08:51
@sundowndev sundowndev self-assigned this Nov 25, 2021
Copy link
Copy Markdown
Contributor

@sundowndev sundowndev left a comment

Choose a reason for hiding this comment

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

Hey @smaftoul, thanks for this first contribution! 🎉

About testing, you need to update the tests so the message only show up when deep mode is enabled. Also don't forget to set the analysis options from the Analyzer. If you need any help, feel free to join our Discord and talk with us, we'd be happy to provide some help.

@smaftoul smaftoul force-pushed the fix/ouf-of-sync-only-deep-mode branch from 9aa787f to 7476cfa Compare November 25, 2021 11:56
@smaftoul
Copy link
Copy Markdown
Contributor Author

About testing, you need to update the tests so the message only show up when deep mode is enabled. Also don't forget to set the analysis options from the Analyzer.

I'm not sure how I should modify tests, I think the fakeAnalysis doesn't have an Analyzer, so we cannot gather it's deepoption. Is this right ?
The tests ends up by failing all outputs and always complaining about the "out of sync" line, because it's never considered as a deep analysis.

@sundowndev sundowndev added the kind/enhancement New feature or improvement label Nov 25, 2021
@smaftoul
Copy link
Copy Markdown
Contributor Author

smaftoul commented Nov 25, 2021

As discussed on discord, I will add an opts argument to the fakeAnalysis() and pass it to the Analysis so it inherits the options without needing to create an Analyzer.

@smaftoul smaftoul force-pushed the fix/ouf-of-sync-only-deep-mode branch from be89683 to 9813fec Compare November 25, 2021 15:58
@smaftoul smaftoul force-pushed the fix/ouf-of-sync-only-deep-mode branch from 9813fec to 8d33187 Compare November 25, 2021 16:07
@sundowndev sundowndev changed the title fix: display out of sync drifts only in deep mode Display out of sync drifts only in deep mode Nov 25, 2021
Copy link
Copy Markdown
Contributor

@sundowndev sundowndev left a comment

Choose a reason for hiding this comment

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

Thank you! 🎉

@sundowndev sundowndev merged commit 383ae5d into snyk:main Nov 26, 2021
@eliecharra
Copy link
Copy Markdown
Contributor

🎉 @all-contributors please add @smaftoul for code

@allcontributors
Copy link
Copy Markdown
Contributor

@eliecharra

I've put up a pull request to add @smaftoul! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement New feature or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants