Skip to content

Conversation

@Emantor
Copy link
Member

@Emantor Emantor commented Apr 30, 2018

Commit 1deea1c introduced a bug where the
internal _run_check function used the run function guarded by a check_active
decorator. Use _run for the run_check function, refactor _run functions for
drivers who did not have it and adjust the tests to mock the correct funtion.

Fixes #230

Signed-off-by: Rouven Czerwinski r.czerwinski@pengutronix.de

@Emantor Emantor added the fix label Apr 30, 2018
@Emantor Emantor requested a review from jluebbe April 30, 2018 10:23
@codecov-io
Copy link

codecov-io commented Apr 30, 2018

Codecov Report

Merging #237 into master will increase coverage by <.1%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #237     +/-   ##
========================================
+ Coverage    54.8%   54.9%   +<.1%     
========================================
  Files         106     106             
  Lines        6267    6273      +6     
========================================
+ Hits         3440    3447      +7     
+ Misses       2827    2826      -1
Impacted Files Coverage Δ
labgrid/driver/smallubootdriver.py 42.4% <ø> (ø) ⬆️
labgrid/driver/shelldriver.py 27.5% <100%> (+0.3%) ⬆️
labgrid/driver/bareboxdriver.py 61.6% <100%> (+0.7%) ⬆️
labgrid/driver/sshdriver.py 49% <100%> (+1%) ⬆️
labgrid/driver/commandmixin.py 80% <100%> (+0.8%) ⬆️
labgrid/driver/ubootdriver.py 47.7% <100%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2a59e2...a3b02fd. Read the comment docs.

@Emantor Emantor force-pushed the fix/_run_function branch from 2a60252 to 4bc771d Compare April 30, 2018 13:04
@Emantor Emantor force-pushed the fix/_run_function branch from 4bc771d to e568fa0 Compare June 4, 2018 09:47
def run(self, cmd: str, *, step, timeout: int = 30):
return run(cmd, step=step, timeout=timeout)

@step(args=['cmd'])
Copy link
Member

Choose a reason for hiding this comment

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

Steps on internal methods aren't really useful.

def run(self, cmd, codec="utf-8", decodeerrors="strict"):
return _run(cmd, codec=codec, decodererrors=decodeerrors)

@step(args=['cmd'])
Copy link
Member

Choose a reason for hiding this comment

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

Here as well.

@Emantor
Copy link
Member Author

Emantor commented Jun 4, 2018

Added a commit which removes the step decorators from _run functions. FWIW: there are more cases where we have step decorators on internal functions, but thats something for another PR.

Edit: rebased on master

@Emantor Emantor force-pushed the fix/_run_function branch from e1c7233 to c28fc9e Compare June 4, 2018 13:58
@Emantor Emantor force-pushed the fix/_run_function branch from c28fc9e to 5675257 Compare July 2, 2018 10:37
@Emantor
Copy link
Member Author

Emantor commented Jul 2, 2018

I updated this branch and fixed the conflicts.

Emantor and others added 3 commits July 3, 2018 13:00
Commit 1deea1c introduced a bug where the
internal _run_check function used the run function guarded by a check_active
decorator. Use _run for the run_check function, refactor _run functions for
drivers who did not have it and adjust the tests to mock the correct funtion.

Fixes #230

Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
The _run functions are called through public functions which have a step
decorator and just pass the arguments.

Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
@jluebbe jluebbe force-pushed the fix/_run_function branch from 5675257 to a3b02fd Compare July 3, 2018 11:01
@jluebbe
Copy link
Member

jluebbe commented Jul 3, 2018

I've reworked the patches a bit (fix infinite recursion, move steps instead of deleting) and added tests for run() in addition to run_check().

@kjeldflarup
Copy link
Contributor

I just found the need to add codec="utf-8", decodeerrors="strict" to run_check, but a PR on that would clearly clash with this PR.
Hope that this PR will be ready soon.

@jluebbe jluebbe merged commit 77a5b98 into labgrid-project:master Jul 3, 2018
@jluebbe
Copy link
Member

jluebbe commented Jul 3, 2018

@kjeldflarup Would having a run_bytes() which worked with bytes instead of strings help you? Then you could handle the decoding youself.

@kjeldflarup
Copy link
Contributor

@jluebbe We already have codecs on run. I just got the nice idea to use run_check as much as possible.
I don't think it is worth it to invent more methods than necessary.

@jluebbe
Copy link
Member

jluebbe commented Jul 3, 2018

I'd like to understand your usecase better:
How does that look in you test cases? Do you use different codes and error settings on one target depending on the test? If it's always the same, would a default codec at the ShellDriver be more useful?

Bastian-Krause added a commit to Bastian-Krause/labgrid that referenced this pull request Jul 3, 2018
_run() methods are not called with step and step is not used anyway, so
remove it.

This fixes errors like:

  TypeError: _run() missing 1 required keyword-only argument: 'step'

Issue introduced by PR labgrid-project#237.

Signed-off-by: Bastian Stender <bst@pengutronix.de>
jluebbe pushed a commit that referenced this pull request Jul 3, 2018
_run() methods are not called with step and step is not used anyway, so
remove it.

This fixes errors like:

  TypeError: _run() missing 1 required keyword-only argument: 'step'

Issue introduced by PR #237.

Signed-off-by: Bastian Stender <bst@pengutronix.de>
@kjeldflarup
Copy link
Contributor

We had the issue before: #203, perhaps reopen.
Adding codec to run gave a useful solution. Adding it to run_check seems like a natural completion of this path.
Changing in the driver ressources also was a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants