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

Allow multiple URL parameters in auto tests endpoints. #851

Conversation

jonathan-r-thorpe
Copy link
Contributor

  • This removes the current limitation of a single URL parameter in an endpoint URL when running auto tests.
  • This is a problem for the emerging IS-14 API as this has endpoints with two URL parameters.
  • This code is generalized to allow any number of URL parameters, simplifying the existing implementation.

@garethsb
Copy link
Contributor

This is awesome, no doubt... one of those cases where it feels like test code needs tests to ensure no regression, or at least maybe a demo on all the existing parameterized APIs that the same coverage is produced by the new implementation, maybe?

@jonathan-r-thorpe
Copy link
Contributor Author

Thanks @garethsb! I'm going to modify the code so the error message format is identical to the original code: this code just reports the path with the parameters embedded, whereas the original code reports the parameter values only. (In the case of no parameters, including the path seems a bit redundant).

I was then going to do an A/B comparison for each of the test suites against nmos-cpp's Node and Registry. Assuming there are no differences would that be sufficient to prove no regressions?

…test result for paths dependent on failed parameterized endpoints.
@garethsb
Copy link
Contributor

Sounds great to me! Can check all IS-04 APIs, IS-05, IS-07, IS-08, this way. I guess you may want to do one test with non-zero MAX_TEST_ITERATIONS too?

@jonathan-r-thorpe
Copy link
Contributor Author

I can confirm that the auto tests run identically with this PR. I ran in both secure and authorized mode, with MAX_TEST_ITERATIONS both set and unset. I ran, and compared the following test suites, all of which gave identical results:

  • IS-04-01
  • IS-04-02
  • IS-04-03
  • IS-05-01
  • IS-05-02
  • IS-07-01
  • IS-07-02
  • IS-08-01
  • IS-08-02

The only difference were in IS-04-02, the PR "revealed" a couple of "hidden endpoints" in the registration API that had two parameters in them (see images below).

In summary, I didn't find any regressions.

Original test suite results (partial)
IS-04-02_orig

After the PR is applied, "auto_registration_5" and "auto_registration_6" are "revealed". However no resources to test them :(
IS-04-02_mod

Copy link
Contributor

@garethsb garethsb left a comment

Choose a reason for hiding this comment

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

Looks better than good to me! Thank you, @jonathan-r-thorpe

@jonathan-r-thorpe jonathan-r-thorpe merged commit 13d8835 into AMWA-TV:master Apr 25, 2024
1 check passed
@jonathan-r-thorpe jonathan-r-thorpe deleted the auto-test-multiple-parameters branch April 25, 2024 10:28
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.

2 participants