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

[P4Testgen] Introduce a configuration structure for the test back ends. #4372

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Jan 29, 2024

This pull request adds the TestBackendConfiguration object to P4Testgen. This object contains configuration options which are consumed by the test back ends. Most of the changes involve refactoring.

Currently, the object has the testname, seed, the test path and the amount of tests to generate as inputs.

@fruffy fruffy marked this pull request as draft January 29, 2024 15:17
@fruffy fruffy force-pushed the fruffy/testbackend_config branch 2 times, most recently from c14bb75 to 4aa0729 Compare January 29, 2024 20:04
@fruffy fruffy added the p4tools Topics related to the P4Tools back end label Jan 29, 2024
@fruffy fruffy force-pushed the fruffy/protobuf_p4runtime branch 3 times, most recently from f749472 to ac8f288 Compare February 1, 2024 14:12
@fruffy fruffy force-pushed the fruffy/protobuf_p4runtime branch 3 times, most recently from ef11adf to 107d409 Compare February 2, 2024 22:32
@fruffy fruffy force-pushed the fruffy/protobuf_p4runtime branch 2 times, most recently from 3edd94b to 6d78adc Compare February 8, 2024 00:10
Base automatically changed from fruffy/protobuf_p4runtime to main February 8, 2024 02:42
@fruffy fruffy force-pushed the fruffy/testbackend_config branch 3 times, most recently from 38da2dc to d2a5bb4 Compare February 8, 2024 13:29
@fruffy fruffy changed the title [P4Testgen] Introduce a configuration structure for the test back ends. Add the option to customize test names. [P4Testgen] Introduce a configuration structure for the test back ends. Feb 8, 2024
@fruffy fruffy marked this pull request as ready for review February 8, 2024 13:33
auto optBasePath = getTestBackendConfiguration().fileBasePath;
BUG_CHECK(optBasePath.has_value(), "Base path is not set.");
auto incrementedbasePath = optBasePath.value();
incrementedbasePath.concat("_" + std::to_string(testId));
incrementedbasePath.replace_extension(".txtpb");
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to lift this into a lib function (of the config?), parametrized by the test id and extension.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an annoyingly common pattern with std::optional and std::expected. Makes you wish C++ had a ? operator.

I have been thinking about introducing https://github.com/TartanLlama/optional at some point, since it fills the gap std::optional left with optional references and unpacking.
Or simply use some custom macro like this

#define ASSIGN_OR_RETURN_WITH_MESSAGE_IMPL(temporary, targetVariable, inputFunction, returnValue, \
                                           errorFunction)                                         \
    auto(temporary) = inputFunction;                                                              \
    if (!(temporary)) {                                                                           \
        errorFunction;                                                                            \
        return returnValue;                                                                       \
    }                                                                                             \
    targetVariable = *(temporary);  // NOLINT

#define ASSIGN_OR_RETURN_WITH_MESSAGE(targetVariable, inputFunction, returnValue, errorFunction) \
    ASSIGN_OR_RETURN_WITH_MESSAGE_IMPL(STATUS_MACROS_CONCAT_NAME(temporary, __LINE__),           \
                                       targetVariable, inputFunction, returnValue, errorFunction)

There is also https://github.com/abseil/abseil-cpp/blob/master/absl/status/statusor.h.

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 have created an issue to track this #4403.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the general case is interesting too, but here I meant the function to derive test file name from configuration, which is almost exactly the same in most testgen backend.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, there are more patterns that are similar in the test back ends. Any back end that writes to files and does not use a preamble (PTF) has a similar pattern. I could move the entire sequence into a helper.

However, I would like to clean up the error handling first before introducing a new utility function.

@fruffy fruffy merged commit 04fc28a into main Feb 8, 2024
16 checks passed
@fruffy fruffy deleted the fruffy/testbackend_config branch February 8, 2024 23:08
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