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

[P4Tools] Clean up the P4Tools GTest infrastructure. #4841

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Jul 31, 2024

Clean up some of the P4Testgen Gtest infrastructure

  • Wrap everything in the P4Tools::Test namespace to reduce some namespace clutter.
  • Introduce gtest_utils in the BMv2 test folder. Provides some BMv2-specific common definitions. This will be used in subsequent pull requests.
  • Move saturation_arithm in the correct folder. It is part of the Z3 tests.

Prerequisite for #4787.

@fruffy fruffy added the p4tools Topics related to the P4Tools back end label Jul 31, 2024
@fruffy fruffy force-pushed the fruffy/p4tools_test_cleanup branch 4 times, most recently from a518cd4 to 54528ee Compare August 1, 2024 07:08
@fruffy fruffy marked this pull request as ready for review August 1, 2024 08:04
@fruffy fruffy requested a review from vlstill August 1, 2024 08:04
@fruffy fruffy force-pushed the fruffy/p4tools_test_cleanup branch from 54528ee to 368e18f Compare August 15, 2024 08:20
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Few small things...

using namespace P4::literals;

class P4AssertsParserTest : public P4ToolsTest {};
class P4AssertsParserTest : public ::testing::Test {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a conflict on the testing namespace? Or why use the global marker ::?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this is an artifact of other tests using ::testing::Test and things getting copied as needed.
Eg., https://github.com/p4lang/p4c/blob/main/test/gtest/bitrange.cpp#L37

I can clean these up to testing::Test, if desired.

@fruffy fruffy requested a review from vlstill August 19, 2024 15:01
@fruffy fruffy added this pull request to the merge queue Aug 27, 2024
@fruffy fruffy removed this pull request from the merge queue due to a manual request Aug 27, 2024
fruffy added 2 commits August 27, 2024 14:07
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
@fruffy fruffy force-pushed the fruffy/p4tools_test_cleanup branch from 51dcdb2 to 4957d6f Compare August 27, 2024 12:07
@fruffy fruffy enabled auto-merge August 27, 2024 12:53
@fruffy fruffy added this pull request to the merge queue Aug 27, 2024
Merged via the queue into main with commit 0aa87b3 Aug 27, 2024
18 checks passed
@fruffy fruffy deleted the fruffy/p4tools_test_cleanup branch August 27, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4tools Topics related to the P4Tools back end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants