Skip to content

Conversation

wader
Copy link
Collaborator

@wader wader commented Aug 14, 2022

google/gopacket#933 rebased, regenerated with linter and benchmark commits skipped as i'm not sure they are relevant anymore.

I did a similar PR google/gopacket#1015 sometime ago before i found the one by @dylandreimerink

Done rudimentary manual testing of pcaps. Should some kind of test(s) be added?

@mosajjal
Copy link
Contributor

just converted some of the travis checks into Github ones. gonna close and reopen this to trigger it :)

@mosajjal mosajjal closed this Aug 16, 2022
@mosajjal mosajjal reopened this Aug 16, 2022
@wader
Copy link
Collaborator Author

wader commented Aug 16, 2022

@mosajjal 👍 no worries

@mosajjal
Copy link
Contributor

looks good and doesn't break any existing code. However, it doesn't look like it has any tests. If you have any tests ready to commit that'd be good.

@wader
Copy link
Collaborator Author

wader commented Aug 16, 2022

There was no tests in the original PR but i can try add something. What you be a good test, parse some minimal sll2 pcap test file?

@mosajjal
Copy link
Contributor

take a look at layers tests like dns. For the sake of consistency we can dump the pcap into bytes for testing purposes. makes it easier imo

@wader
Copy link
Collaborator Author

wader commented Aug 16, 2022

Added a test case but only included bytes from sll2 and down, looked like other tests only tested from their layer and down.

@mosajjal mosajjal merged commit 60750a3 into gopacket:master Aug 16, 2022
@mosajjal
Copy link
Contributor

Yep I gather it's the same. looks good to me :) merged

@wader
Copy link
Collaborator Author

wader commented Aug 16, 2022

Confused, what is this about Go code is not formatted, run 'go fmt github.com/google/stenographer/...? should say go fmt ./...?

A did a fmt and lots of things changed... but i gess you have a PR for this?

@wader
Copy link
Collaborator Author

wader commented Aug 16, 2022

@mosajjal Aha, good thanks!

@wader wader deleted the sll2 branch August 16, 2022 11:17
@mosajjal
Copy link
Contributor

yep that's an old relic from the Google's repo and Travis CI stuff. hopefully all will get removed in a couple days :)

@wader
Copy link
Collaborator Author

wader commented Aug 16, 2022

👍 Great, and thanks for picking up the gopacket development again!

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