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

Use test tools from win-net-test #477

Merged
merged 14 commits into from
Mar 28, 2024
Merged

Use test tools from win-net-test #477

merged 14 commits into from
Mar 28, 2024

Conversation

nigriMSFT
Copy link
Contributor

Remove the xdpfnmp, xdpfnlwf and pkthlp test components and consume their equivalents from the win-net-test project.

Resolves #422

@nigriMSFT nigriMSFT requested a review from a team as a code owner March 26, 2024 22:26
@@ -143,6 +143,28 @@ function Get-EbpfPackageUrl {
return "https://github.com/microsoft/ebpf-for-windows/releases/download/Release-v" + $EbpfVersion + "/" + $EbpfPackageName
}

function Get-FnVersion {
return "0.4.1"
Copy link
Member

Choose a reason for hiding this comment

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

This seems painful to keep in-sync with the dependency. Is there no better way to have this somewhere more easily found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 100% agree. This is the pattern used for the eBPF dependency though (and the pattern used by the wtc project's consumption of fnmp), so its no worse off than how existing dependencies are handled.

Copy link
Member

Choose a reason for hiding this comment

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

Alright. We can keep it as is for now. But I'm not a big fan. I'd prefer some top-level json file or something that lists the dependencies and version numbers. We can follow up later though.

nibanks
nibanks previously approved these changes Mar 26, 2024
@nigriMSFT
Copy link
Contributor Author

Hmm the "Downlevel Functional Tests" are running a downlevel version of the test, but with the uplevel version of the test tool installation. This appears to cause an issue, since the test logic is hardcoded to expect the test adapter to have a certain InterfaceDescription/ifdesc (XDPFNMP) and the test adapters are now installed with a difference ifdesc (FNMP).

@nibanks
Copy link
Member

nibanks commented Mar 26, 2024

Hmm the "Downlevel Functional Tests" are running a downlevel version of the test, but with the uplevel version of the test tool installation. This appears to cause an issue, since the test logic is hardcoded to expect the test adapter to have a certain InterfaceDescription/ifdesc (XDPFNMP) and the test adapters are now installed with a difference ifdesc (FNMP).

Hmmm. How hard would it be to update the downlevel tests to allow for both? We then have to push a change to downlevel to fix.

@nigriMSFT
Copy link
Contributor Author

I might be able to change the ifdesc with a bit of a hack. Let me try that first

@nigriMSFT
Copy link
Contributor Author

Darn the hack in mind doesn't seem to work.

I guess I can open a PR on the release 1.0 branch. The change should be pretty small and scoped to test code only.

@nigriMSFT
Copy link
Contributor Author

nigriMSFT commented Mar 27, 2024

Hmm the "Downlevel Functional Tests" are running a downlevel version of the test, but with the uplevel version of the test tool installation. This appears to cause an issue, since the test logic is hardcoded to expect the test adapter to have a certain InterfaceDescription/ifdesc (XDPFNMP) and the test adapters are now installed with a difference ifdesc (FNMP).

It is actually not as simple as a difference in ifdesc. The mp device name has also churned and the downlevel test code is compiled with xdpfnmpapi.lib which uses the old device name to communicate with the driver (xdpfnmp vs. fnmp). :(

Maybe we should just merge this whole PR into downlevel as well?
Or think about if the relevant test tools can be archived with the test dll? That route seems more confusing and error prone though.

@nibanks nibanks merged commit 11f6d1b into main Mar 28, 2024
45 checks passed
@nibanks nibanks deleted the nigriMSFT/fntools branch March 28, 2024 19: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.

Replace XDPFNMP with FNMP
2 participants