Makefile.include: add cleanterm target and use it for tests#12107
Merged
benpicco merged 7 commits intoRIOT-OS:masterfrom Oct 1, 2019
Merged
Makefile.include: add cleanterm target and use it for tests#12107benpicco merged 7 commits intoRIOT-OS:masterfrom
benpicco merged 7 commits intoRIOT-OS:masterfrom
Conversation
jcarrano
added
Area: build system
Contributor
Author
|
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) |
Contributor
There was a problem hiding this comment.
should we keep on repeating bad patterns? I'm referring to checking the command.
Contributor
Author
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.
Contributor
|
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. |
Contributor
Author
cladmi
force-pushed
the
pr/tests/cleanterm
branch
from
f8fbe9e to
111d29b
Compare
Contributor
Author
|
Rebased as #12120 was merged. |
This was referenced Sep 10, 2019
jcarrano
added
CI: ready for build
Contributor
Author
|
The compilation failures are |
Contributor
|
Works as advertised:
|
fjmolinas
added
the
Reviewed: 3-testing
Contributor
|
I personally like the change, I think the implementation should indeed be split from its usage in |
Contributor
|
Also if the usage in test is included it should be documented in |
Contributor
Author
|
For me the goal of this PR is usage in All tests in CI already use
I will add something there :) |
fjmolinas
added
CI: run tests
fjmolinas
reviewed
Oct 1, 2019
Contributor
fjmolinas
left a comment
fjmolinas
left a comment
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
Contributor
|
Looks good. Just found a minor typo. Please squash @cladmi ! |
Contributor
|
No changes in tests results. There is a new test failure for |
cladmi
force-pushed
the
pr/tests/cleanterm
branch
from
f266b69 to
af260ae
Compare
Contributor
Author
|
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.
cladmi
force-pushed
the
pr/tests/cleanterm
branch
from
af260ae to
66300b2
Compare
Contributor
Author
|
I would say, enable running all tests without caching, as murdock will not re-run tests on this change. |
Contributor
Author
|
The |
fjmolinas
added
CI: disable test cache
Contributor
|
Murdock says OK after ¾h of labor |
Contributor
Author
|
Tests are also working locally on |
Contributor
Author
|
Thank you for the review. Now we can correctly get the return value of a shell command !!!! 🎉 |
1 task
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.