-
Notifications
You must be signed in to change notification settings - Fork 477
[docs] Added test guide #5215
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
[docs] Added test guide #5215
Conversation
Note: Changes may take a few seconds to appear on GitHub Pages. Please refresh the page if you do not see the updates immediately. |
|
@jafingerhut there is no bmv_errors_output directory in the testdata |
That is true. There is also no directory |
If you mean: "Why is there a directory named p4_16_bmv_errors in the testdata directory, but there is no corresponding expected outputs directory named p4_16_bmv_errors_output?" then my answer is "I don't know". It would be good to find out, and either correct the problem, or document why one is not necessary. It might be a mistake that there is no directory p4_16_bmv_errors_output. |
|
yes I mean that why there is bmv_errors file but no output No problem I will try to find out why there is no output file
|
|
@jafingerhut can you please review this |
docs/AddingTestGuide.md
Outdated
|
|
||
| ## Running Tests | ||
|
|
||
| - To run all tests: `make check -j3` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not everyone uses make. I would recommend ctest here. If make is recommended somewhere else, I would update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added testing using ctest section and remove cmake one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also running test using make is written in the p4 docs so if you want i can update the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please update the docs. But this section also should be fixed. ctest is how tests are run, not make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, this is how you would run tests using the same command cmake --build build --target check -- -j3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
docs/AddingTestGuide.md:17
- The source file location for Negative Tests has been removed without a replacement. Please verify if updated directory paths should be provided to ensure clarity for readers.
- `testdata/p4_16_errors/`
docs/AddingTestGuide.md:28
- The Backend-Specific Tests section no longer lists expected source file locations. If these paths have changed, consider updating this section with the current directories to avoid confusion.
- `backends/ebpf/tests/`
docs/AddingTestGuide.md
Outdated
| Positive tests are standard tests where the compiler should not fail due to compile-time errors. The expected output should match the actual output for the test to pass.The compiler should generate a output file with no errors. | ||
|
|
Copilot
AI
Apr 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a missing space after the period in 'pass.The'; please add a space to improve readability.
| Positive tests are standard tests where the compiler should not fail due to compile-time errors. The expected output should match the actual output for the test to pass.The compiler should generate a output file with no errors. | |
| Positive tests are standard tests where the compiler should not fail due to compile-time errors. The expected output should match the actual output for the test to pass. The compiler should generate a output file with no errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fruffy can you please review it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
docs/AddingTestGuide.md:40
- [nitpick] The sentence seems a bit awkward; consider rephrasing it for clarity, for example: 'Similarly, for other error output files, please refer to the corresponding directories.'
Similarly for other error output files you can check
docs/AddingTestGuide.md:25
- [nitpick] Consider capitalizing 'ubpf' to 'UBPF' for consistency with 'eBPF' to improve clarity.
Some test programs exist in backend-specific directories, such as eBPF, ubpf tests.
|
@fruffy You mentioned in an earlier comment: "Yes, please update the docs. But this section also should be fixed. ctest is how tests are run, not make." I searched through existing p4c files (latest committed as of 2025-Jun-17), and there are quite a few places that mention commands with I have used Are you hoping to deprecate |
Well, I rushed this comment out a bit, but the point is that many are building p4c using The recommended way to do this is to use the cmake front end with |
|
@Vineet1101 If you have a bit of time, could you take a look at Fabian’s comment so we can get the PR merged? It would be a pity to keep this work pending after all the effort you’ve put into it |
docs/AddingTestGuide.md
Outdated
| - Tests targeting eBPF should end with -ebpf.p4. | ||
| - General frontend tests should use descriptive names that reflect the feature being tested. | ||
| 3. Generate reference outputs: | ||
| - For frontend tests: `../backends/p4test/run-p4-sample.py . -f ../testdata/p4_16_samples/some_name.p4` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The easiest way to (re)generate expected outputs for tests is to set P4TEST_REPLACE=1 in the environment before running the test(s), and then run the tests however you normally do (ctest or make check or however you make ninja run them, or even just running .test scripts directly)
| #### Source File Location | ||
| - Various locations within `testdata/` | ||
|
|
||
| ## Running Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way the test suite is designed, is that cmake will create many .test scripts in the build tree (in subdirectoroes named for the target and the test source directory -- eg build/bmv2/testdata/p4_16_samples) that runs that each run a single test, and sets up ctest to run all those scripts. So you can run individual tests by just running a .test script directly from the command line, or you can run ctest, or you can run a build tool to run ctest (eg make check)
|
@Dscano @fruffy @jafingerhut I will complete this by the end of the day |
Signed-off-by: Vineet1101 <Vineetgoel692@gmail.com>
Signed-off-by: Vineet1101 <Vineetgoel692@gmail.com>
Signed-off-by: Vineet1101 <Vineetgoel692@gmail.com>
Signed-off-by: Vineet1101 <Vineetgoel692@gmail.com>
72ecc79 to
124c064
Compare
|
@jafingerhut @fruffy can you please review it I have made changes as per your review |
fruffy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jafingerhut Any other comments?
Signed-off-by: Vineet1101 <Vineetgoel692@gmail.com>
This PR adds a comprehensive guide on testing in the P4C repository. The document covers different types of tests, their expected behavior, source file locations, how to run tests, and how to add new tests. Additionally, it includes details on continuous integration and automated test execution.