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

Remove dont_skip_tests feature #27

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

danielocfb
Copy link
Collaborator

@danielocfb danielocfb commented Dec 8, 2022

We don't need a feature if we don't want to skip a test based on a runtime condition. We can just make it an ignored one, in which case it can be run with cargo test -- --ignored.
Hence, remove the feature and mark the one test using it as ignored instead.

Signed-off-by: Daniel Müller deso@posteo.net

@ThinkerYzu1
Copy link
Collaborator

What is the version of the cargo you use? I run v1.65.0 that doesn't support --ignored.

@danielocfb
Copy link
Collaborator Author

danielocfb commented Dec 8, 2022

What is the version of the cargo you use? I run v1.65.0 that doesn't support --ignored.

It's an option of the test. You can try cargo test -- --ignored.

@danielocfb
Copy link
Collaborator Author

What is the version of the cargo you use? I run v1.65.0 that doesn't support --ignored.

It's an option of the test. You can try cargo test -- --ignored.

Sorry I wrote it wrong in the description. Will update. But actually stuff is failing now. Will fix it.

@danielocfb
Copy link
Collaborator Author

But actually stuff is failing now. Will fix it.

So the problem is that once we try to use ignored tests, the snippets from the README are compiled and they are broken right now. So for the time being I am patching them up quickly, but really we should fix the outdated stuff in there and document the current API.

@ThinkerYzu1
Copy link
Collaborator

ThinkerYzu1 commented Dec 8, 2022

Some snippets are supposed to be incomplete, and some of them are C code. Should we tag them as text?

@danielocfb
Copy link
Collaborator Author

Some snippets are supposed to be incomplete. Should we just tag them as text?

They are actually broken. SymSrcCfg has been renamed to SymbolSrcCfg, for example. Will look into it more closely as part of #32, but I think for now what I did is reasonable. Let me know if you disagree.

@ThinkerYzu1
Copy link
Collaborator

ThinkerYzu1 commented Dec 8, 2022

What I am saying is that some snippets are supposed to be incomplete. And some of them are C examples. Although some snippets can be compiled, but these codes are not supposed to be tested for lack of the right environment. For example, it symbolizes a random address in an example. A random address is not going to work. So, they are ignored for good reasons. So, what is the solution for these cases? Tag them as text?

@danielocfb
Copy link
Collaborator Author

What I am saying is that some snippets are supposed to be incomplete. And some of them are C examples. Although some snippets can be compiled, but these codes are not supposed to be tested for lack of the right environment. For example, it symbolizes a random address in an example. A random address is not going to work. So, they are ignored for good reasons. So, what is the solution for these cases? Tag them as text?

Well, I think you are conflating compiling them and running them. Nobody is talking about running anything. Yes, I can see that some are not meant to be compiled. And yet, they should not reference obviously non-existent symbols, should they? That is confusing at best to anyone trying out the crate based on the README and should be fixed up. C language snippets should be fixed in exactly the way I did fix them: by tagging them as C.

@danielocfb
Copy link
Collaborator Author

Okay, I fixed up everything now. Let me know what you think.

@ThinkerYzu1
Copy link
Collaborator

ThinkerYzu1 commented Dec 9, 2022

they should not reference obviously non-existent symbols, should they?

No one said that. I just asked what your plan was. It doesn't work without the changes that you just did. You just need to tell me your plan or just show me the changes.

Copy link
Collaborator

@ThinkerYzu1 ThinkerYzu1 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 for fixing these issues.

We don't need a feature if we don't want to skip a test based on a
runtime condition. We can just make it an ignored one, in which case it
can be run with `cargo test -- --ignored`.
Hence, remove the feature and mark the one test using it as ignored
instead. Note that currently the README contains broken code snippets. I
fixed it up to at least allow running of ignored tests, but it really
should be updated to use the up-to-date API.

Signed-off-by: Daniel Müller <deso@posteo.net>
The code snippets in the README are outdated. The SymSrcCfg type
mentioned there, for example, seems to have been renamed in the library
to SymbolSrcCfg eons ago. Update the README accordingly. Also tag Rust
code snippets as Rust code and C snippets as C. That has both the effect
that it will ensure proper handling of doc tests (C snippets are
ignored) while providing correct syntax highlighting. Note that not all
snippets can (or are intended to) compile, but those that can are fully
fixed up now and checked as part of CI by tagging them with `no_run`
instead of `ignore`.

Closes: libbpf#32

Signed-off-by: Daniel Müller <deso@posteo.net>
@danielocfb danielocfb merged commit c713d15 into libbpf:master Dec 9, 2022
@danielocfb danielocfb deleted the topic/ignore-test branch December 9, 2022 20:01
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.

None yet

3 participants