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

sys/test_utils/result_output: Initial implementations and API of turo #15950

Merged
merged 8 commits into from
Mar 15, 2021

Conversation

MrKevinWeiss
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss commented Feb 8, 2021

Contribution description

The latest in an effort to add an abstraction layer for RIOT tests allowing a more stable testing API.
This focuses more on how results are printed or output.
Using the results_output API will allow a guaranteed way output data, meaning no more needing to adapt regex everytime a printf is changed.
The exact output can be changed depending on the module selected, this means if you don't like it you can just implement a variant that suits your needs.
The eventual goal will be to coordinate this with riotctrl so data can be used immediately when writing python based automated tests.

There are currently 4 variant implemented:

  • json: used for CI and computer interaction
  • txt: used for developer interaction
  • check: Only ensuring the output follows a correct structure
  • led: An example of using the turo_t *ctx to flash LEDs

More information is in the sys/test_utils/results_output.h file.

Note: The test_utils_result_output_json is not fully json compliant as it contains trailing commas. This is allows simple flushing of information and no need for states. Many parses accept trailing commas such as rapidjson python parser.

Testing procedure

OUTPUT_FORMAT={txt, check, json, led} make all -C tests/turo

note: native has "leds" but they don't fit the LED0_PIN define so an assertion error will be thrown because native uses GPIO_UNDEF

Issues/PRs references

supersedes #15039
also supersedes #11224
Implementation of a cleaner idea of #10624

@MrKevinWeiss
Copy link
Contributor Author

MrKevinWeiss commented Feb 9, 2021

TODO:

  • Add module to uncrustify?
  • Add kconfig options
  • Add unittest to get full coverage (I think I don't want to complicate the test/example too much)

@MrKevinWeiss
Copy link
Contributor Author

All that is done, I probably should split this PR up. @leandrolanzieri thanks for the kconfig help

@aabadie
Copy link
Contributor

aabadie commented Feb 9, 2021

Do you really expect that people who write test applications will use turo_ calls instead of printf ? Do you think this makes the code of "tests used as examples" helpful ?

@MrKevinWeiss
Copy link
Contributor Author

Do you really expect that people who write test applications will use turo_ calls instead of printf ? Do you think this makes the code of "tests used as examples" helpful ?

I have been asking about this for a while... For simple things maybe not (hence optional) but if I am writing a more complex test that I uses a matching python module for might be useful.

Does it make sense to try to enforce a structure when writing tests?

For the tests or modules that use, say json strings, would it not be better to use some common functions instead of always printf("{\"my_escape_char\": \"\\\"}\n")

@aabadie
Copy link
Contributor

aabadie commented Feb 9, 2021

Does it make sense to try to enforce a structure when writing tests?

I agree that in certain situations such a module could be useful: for benchmark tests or very complex tests, as you say. For tests that are just there as usage examples, it's important to keep them as self-explanatory as possible and to limit the usage of test specific modules, eventually they could be moved to the examples directory instead (but in this case, we need a hierarchy of directories instead of the actual flat organization 🤔 ).

@MrKevinWeiss
Copy link
Contributor Author

we need a hierarchy of directories instead of the actual flat organization

I think that has also been discussed quite a bit :)

@aabadie Is that a blocker for you or can we proceed with the concept (again, optional only if it makes sense for that test)?

Comment on lines 2 to 17
* Copyright (C) 2014 Freie Universität Berlin
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/

/**
* @ingroup tests
* @{
*
* @file
* @brief GET_CPU_ID() test application
*
* @author Martine Lenders <[email protected]>
* @author Hauke Petersen <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem to be correct ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be... could be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@MrKevinWeiss
Copy link
Contributor Author

Now the LED module is dripped this is a bit simpler PR. Any remaining objections?

@miri64
Copy link
Member

miri64 commented Mar 11, 2021

#15950 (comment) remains uncommented.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

If this one was addressed, I think we can merge.

*
* Some of the design decisions include:
* - ability to flush immediately to prevent the need for large buffers
* - selectable output format based on USEMODULE
Copy link
Member

Choose a reason for hiding this comment

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

I think this was not addressed yet too.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

This is ready to go in my opinion. I think your changes were addressed too, @kaspar030, right?

@miri64
Copy link
Member

miri64 commented Mar 12, 2021

Pleases squash, @MrKevinWeiss.

@miri64 miri64 added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 12, 2021
@miri64 miri64 dismissed kaspar030’s stale review March 12, 2021 15:28

@kaspar030 gave a thumbs up to my ACK, which I interpret that we can dismiss his review. Please re-request changes, if that is not the case.

@miri64
Copy link
Member

miri64 commented Mar 12, 2021

Sadly, Murdock's not happy :(

@miri64
Copy link
Member

miri64 commented Mar 12, 2021

Took me a while to interpret the output: apparently there are now different binaries generated with and without Kconfig.

@MrKevinWeiss
Copy link
Contributor Author

Hmmm, I will look into it.

@MrKevinWeiss
Copy link
Contributor Author

It seems I struggled with the rebasing and removing of the module. I make the turo module optional in kconfig and default select json.

@MrKevinWeiss
Copy link
Contributor Author

@miri64 I think things are looking good now!

@miri64
Copy link
Member

miri64 commented Mar 15, 2021

🎉 then let's go!

@miri64 miri64 merged commit fd36c62 into RIOT-OS:master Mar 15, 2021
@MrKevinWeiss MrKevinWeiss deleted the pr/turo/initial branch March 15, 2021 11:32
@MrKevinWeiss
Copy link
Contributor Author

Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants