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: fix snapshot tests #277

Merged
merged 1 commit into from
Aug 26, 2023
Merged

fix: fix snapshot tests #277

merged 1 commit into from
Aug 26, 2023

Conversation

sigmaSd
Copy link
Collaborator

@sigmaSd sigmaSd commented Aug 25, 2023

Not sure how it worked before, but the default location of snapshots is "./snapshots" but it currently is inside"../snapshots"

There are 2 ways to fix this, move to the correct folder : this is whats done in this pr, the second way is to use https://docs.rs/insta/latest/insta/struct.Settings.html#method.set_snapshot_path (which should result a way smaller diff) but I'm not sure where to call that

cargo-insta = "1.31.0" is incorrect , since its a binary, the correct way to use it is to use cargo install cargo-insta and then cargo insta review

@sigmaSd
Copy link
Collaborator Author

sigmaSd commented Aug 25, 2023

Don't have windows to debug, seems like it can't find a dll maybe the pcap sdk ?

@cyqsimon
Copy link
Collaborator

Huge thanks for fixing the tests. This was giving me a real headache since I have no clue how it all works.

Don't have windows to debug, seems like it can't find a dll maybe the pcap sdk ?

Yeah I think you are correct. If you take a look at build.rs, pcap has to be explicitly downloaded for bandwhich to compile at all. I suspect we might have to do something similar for the tests, either in the tests themselves or in CI.

I'm not too familiar with Windows development either, so I'll go dig around a bit.

@cyqsimon
Copy link
Collaborator

Alright, I tested it on my Windows VM. In a graphical environment, there's an error popup that says "packet.dll was not found". So yeah, the user needs to have npcap (the binary release!!, not the SDK) installed to run the tests.

But it gets more complicated for CI because only the paid OEM version offers silent install, which is what we need[1] here. Apparently others have run into the same issue (nmap/npcap#227), and it seems like npcap developers are willing to offer their OEM installers to open source projects for CI purposes. So I guess I'll be shooting them an email for this.

[1]: Since a Windows installer executable is just an archive, technically I can cheat and extract the DLL we need manually from the installer of the free version. But obviously that's the last-ditch solution.

@cyqsimon
Copy link
Collaborator

I'll merge this for now, since that Windows CI fix can be its separate thing. Thanks a lot for the contribution.

@cyqsimon cyqsimon merged commit 09b13f3 into imsnif:main Aug 26, 2023
4 of 6 checks passed
@cyqsimon cyqsimon mentioned this pull request Aug 26, 2023
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.

2 participants