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

Update documentation for adding new tests #5113

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Vineet1101
Copy link
Contributor

@Vineet1101 Vineet1101 commented Jan 27, 2025

This PR updates the documentation for adding tests to the P4C repository.


Pull Request Title:

Update Documentation: Guide for Adding New Tests to P4C Repository


Description:

This PR adds a detailed guide to the documentation for developers looking to contribute tests to the P4C repository. The document outlines:

  1. Steps to add positive and negative tests.
  2. Instructions for handling expected output files.
  3. Guidelines for marking tests as "expected to fail" (Xfail).
  4. Considerations for different backends, tools, and test workflows.
  5. Recommendations for running tests via Bash scripts or CTest.

The guide aims to enhance developer productivity and ensure consistency in test contributions.


Motivation:

  • Provide clarity on test organization and expected workflows.
  • Encourage best practices for adding, updating, and running tests.
  • Address gaps in existing documentation, particularly for backend-specific test requirements and Xfail behavior.

Changes Made:

  • Added a comprehensive document titled "Guide for Adding New Tests" under the docs directory.
  • Updated information on BMv2-specific file naming patterns and the behavior of Xfail tests in CI pipelines.

Testing:

  • Verified the document against existing repository structures and workflows.
  • Conducted a review to align with current testing practices and CI configurations.

Checklist:

  • Documentation adheres to repository contribution guidelines.
  • Added information is consistent with existing test workflows.
  • Verified that all details are applicable to the current repository structure.

Related Issues:

None directly. This PR aims to improve developer experience and test consistency proactively.


@fruffy fruffy added the documentation Topics related to compiler documentation. label Jan 27, 2025
Copy link
Contributor

github-actions bot commented Jan 27, 2025

githubloading A preview of this PR is available at: Preview Link
📂 View the source code here: View Source Code
🔧 Commit used for deployment: a99b97dc54966eeb51811c6fe3e396809571939d

Note: Changes may take a few seconds to appear on GitHub Pages. Please refresh the page if you do not see the updates immediately.

@Vineet1101
Copy link
Contributor Author

hey @jafingerhut can you please review this pr


## **4. Marking Tests as "Expected to Fail" (Xfail)**

An Xfail test is one that is expected to fail due to a known issue or limitation in the P4 compiler. The CI system handles these tests differently based on their type (positive or negative).
Copy link
Contributor

Choose a reason for hiding this comment

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

An XFail is a test that should pass (by the language spec and architecture), but currently fails due to known problems in the compiler.


- If a change to the P4 compiler modifies the expected outputs, you can regenerate them by rerunning the tests with:
```bash
./build/p4test path/to/source_file.p4 --update-outputs
Copy link
Contributor

Choose a reason for hiding this comment

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

I can find no mention of update-outputs anywhere in the p4c repo. Where did you find such an option?

@Vineet1101
Copy link
Contributor Author

@jafingerhut @ChrisDodd I have made the changes as per your review. Please review them

```
- To generate or update the expected output file for a single test case, use:
```bash
P4TEST_REPLACE=1 ./build/p4test path/to/source_file.p4
Copy link
Contributor

Choose a reason for hiding this comment

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

I can try out this command this weekend on a personal system of mine, but have you tried it to verify that it actually generates or updates the expected output files? If so, I am surprised, because p4test has nothing built into it to tell which directory to put the expected output files in.

- **`p4_14_samples`**: Contains tests specific to the older P4\_14 language version.
- **`p4_14_samples_outputs`**: Stores the expected outputs for tests in `p4_14_samples`.
- **`bmv2`**: Holds tests targeting the BMv2 backend, including packet-processing logic and examples.
- **`psa`**: Includes tests for the PSA (Portable Switch Architecture) target.
Copy link
Contributor

Choose a reason for hiding this comment

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

PSA is not a target. It is a P4 architecture.

In general, we could make a table with a row each target, and a column for each P4 architecture, and fill in the entries of the table with "yes" if that target has an implementation for that architecture, and "no" if it does not. e.g. the P4 DPDK target implements the PSA and PNA architectures (at least partially), but it does not implement the v1model architecture at all. The BMv2 back end implements the v1model architecture, and it has the beginnings of implementations of the PSA and PNA architectures, but they are missing a lot there.

I don't know off the top of my head which targets (if any) the psa directory has tests for. I haven't looked at it recently, but it would be nice if we can find out and document it a little more precisely here.

- For eBPF-specific tests: `*_ebpf.p4` or `*_ebpf-kernel.p4`.
- For graph backend tests: `*graph*.p4`.
- For UBPF-specific tests: `*_ubpf.p4`.
- For PSA tests targeting BMv2: `*-bmv2.p4`.
Copy link
Contributor

Choose a reason for hiding this comment

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

There are many tests for the BMv2 back end using the v1model architecture that have file names matching *-bmv2.p4. Do you have examples of specific files with names matching that pattern that use the PSA architecture, too? There might be -- I have not checked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just did a quick check -- yes, there are many that use the PSA architecture, but even more that use the v1model architecture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Topics related to compiler documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants