Makefile.include: add cleanterm target and use it for tests #12107
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
In the current implementation, I directly re-used Also, the introduction could be split from the usage in |
jcarrano
reviewed
Aug 29, 2019
| # TERMFLAGS must be exported for `jlink.sh term_rtt`. | ||
| cleanterm: export PYTERMFLAGS += --noprefix | ||
| cleanterm: $(filter flash, $(MAKECMDGOALS)) $(TERMDEPS) | ||
| $(call check_cmd,$(TERMPROG),Terminal program) |
There was a problem hiding this comment.
should we keep on repeating bad patterns? I'm referring to checking the command.
There was a problem hiding this comment.
I still see it as a different task to remove the bad pattern that I did not want to mix with the other change here.
|
Overall, I think this is a good transition towards rawterm. The next step would be to remove all kinds of buffering and we would have arrived at rawterm. |
f8fbe9e to
111d29b
Compare
|
Rebased as #12120 was merged. |
This was referenced Sep 10, 2019
|
The compilation failures are |
|
Works as advertised:
|
|
I personally like the change, I think the implementation should indeed be split from its usage in |
|
Also if the usage in test is included it should be documented in |
|
For me the goal of this PR is usage in All tests in CI already use
I will add something there :) |
fjmolinas
reviewed
Oct 1, 2019
There was a problem hiding this comment.
I think changes make sense and I'm happy with the current status. I would like someone else to take a look and also ACK this, @aabadie could you take a look?
I'll run all tests on a boar using this PR just be sure before ACK.
aabadie
reviewed
Oct 1, 2019
|
Looks good. Just found a minor typo. Please squash @cladmi ! |
|
No changes in tests results. There is a new test failure for |
f266b69 to
af260ae
Compare
|
I will rebase on the last master too, to simplify re-run the other tests like the |
Use PYTERMFLAGS for configuring the terminal.
It is similar to 'term' but with removing possible additional behaviors from pyterm. It currently removes the logging prefix.
Use a clean terminal without added decoration on the output for testing.
Get the output of a one line command without other garbage messages.
Disable repeating the command on empty line to allow sending empty lines.
Test receiving an empty line. It was before not possible with `pyterm` but is fixed by previous commit.
Add some documentation about tests using `cleanterm` and what guarantees it gives.
af260ae to
66300b2
Compare
|
I would say, enable running all tests without caching, as murdock will not re-run tests on this change. |
|
The |
|
Murdock says OK after ¾h of labor |
|
Tests are also working locally on |
|
Thank you for the review. Now we can correctly get the return value of a shell command !!!! 🎉 |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
It is similar to 'term' but with removing possible additional behaviors
from pyterm.
It currently removes the logging decorators and allow sending empty lines.
Use a clean terminal without added decoration on the output for testing.
Review procedure
pytermlogging and special behavior are required features that cannot be removed as said in #12094This introduces a new target that handles it.
This is completely similar to
rawtermfrom #11099 except that it only works around thepytermissues and will do nothing against line buffering.Testing procedure
Run
BOARD=your_board make -C tests/test_tools/ flash testwith different RIOT_TERMINAL, currently supported (not on all boards)pyterm,socat,picocom. If possible one withterm_rtt. But as it is internally usingpytermit will benefit from the changes automatically.RIOT_TERMINAL=pyterm samr21-xproRIOT_TERMINAL=picocom samr21-xproRIOT_TERMINAL=socat samr21-xpronativeOn IoT-LAB it works tooThe configuration is handled by
jlink.sh term_rtt:Tested without a board, we can see the logging format difference from
pyterm.BOARD=hamilton make -C examples/hello-world/ term
BOARD=hamilton make -C examples/hello-world/ cleanterm
term behaves as before
BOARD=samr21-xpro RIOT_CI_BUILD=1 make --no-print-directory -C tests/test_tools/ term
Issues/PRs references
Replacement of #12094
This is completely similar to
rawtermfrom #11099 except that it only works around thepytermissues and will do nothing against line buffering.