Have IO and Exit Code asserts use the same lookup #16
Conversation
pbaughman
force-pushed
the
ISSUE7_simplify_asserts
branch
from
eebc4e5
to
478845a
Compare
There was a problem hiding this comment.
The changes look good to me. There are a lot of style issues if I were reviewing a pr for launch itself (which is not the case here).
I had a few style/nitpick comments inline.
In the end, it's pretty subjective, and so up to you and not a blocker for my 👍 for this review.
@hidmic might have more useful feedback for the actual API design, as he's been working in the launch testing mindset more than me lately.
| @@ -64,7 +64,7 @@ def generate_test_description(ready_fn): | |||
| class TestTerminatingProcessStops(unittest.TestCase): | |||
|
|
|||
| def test_proc_terminates(self): | |||
| self.proc_info.assertWaitForShutdown(process=dut_process, timeout=10) | |||
| self.proc_info.assertWaitForShutdown(proc=dut_process, timeout=10) | |||
There was a problem hiding this comment.
Seems like a step backwards to me...
There was a problem hiding this comment.
Let me clarify, in my opinion we are going from a good argument/variable name (process) to a less good one (proc). Of course that's in my opinion, due to my preference to avoid abbreviation where ever possible. It makes it easier to search for keywords and it helps people for whom English is not their first language.
There might other places where an abbreviation is being used now where it wasn't before, but that's only one I commented on, even though the same logic would apply in each of those cases.
There was a problem hiding this comment.
This is obviously subject and we're inconsistent in the ROS 2 project, because we have things like cmd_args or env. So, it's really just in this case, the change here seems like a step backward, but again it's very subjective.
There was a problem hiding this comment.
It's this way because of flake8 (indirectly). When this was apex_rostest, the argument was 'node'. When I changed everything to be apex_launchtest, node became proc instead of 'process' so I wouldn't have to reformat a bunch of lines that were close to the character limit.
It's good feedback about searching and ESL - I can make everything 'process' instead of 'proc' There were other breaking changes in here, so it doesn't make it any more breaking
|
|
||
| for proc in resolved_procs: # Nominally just one matching proc | ||
| for output in proc_output[proc]: | ||
| if msg in output.text.decode('ascii'): |
There was a problem hiding this comment.
I know this isn't new to the pr, but I am curious why this is needed.
There was a problem hiding this comment.
To be more specific, why 'ascii' is used, rather than passing no arguments (which is something we do in launch), which will get you 'utf-8':
https://docs.python.org/3/library/stdtypes.html#bytes.decode
There was a problem hiding this comment.
This is a leftover from hacking around with the process_io event. You're correct that it should probably be "passing no arguments" I'll open an issue to update this in a different PR
| return list( | ||
| map( | ||
| lambda x: x.action, | ||
| [self._process_name_dict[name][0] for name in self._process_name_dict] |
There was a problem hiding this comment.
What's the reason for the map()? Couldn't you just:
return [processes[0].action for processes in self._process_name_dict.values()]Also, the [some_dict[name]... for name in some_dict] is a bit weird, as opposed to [value... for value in some_dict.values()].
There was a problem hiding this comment.
Yeah, you're right. This is kind of a logic puzzle right now.
- Gave all of these methods the same signature. They can all take ExecuteProcess actions or strings - New util to handle the lookup of process actions in one place
pbaughman
force-pushed
the
ISSUE7_simplify_asserts
branch
from
478845a
to
7ad1693
Compare
|
@wjwwood Comments addressed. |
New file is apex_launchtest/util/proc_lookup.py. All of the other assert methods use this to translate strings into ExecuteProcess actions. AssertInStdout was the most 'advanced' assert, so most of the logic in proc_lookup was taken from there
Bugfix
Closes