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

Add test coverage for substitution edgecases involving E notation #824

Merged

Conversation

danielcranston
Copy link
Contributor

@danielcranston danielcranston commented Feb 1, 2025

I extended some tests while doing some investigations in ros2/launch_ros#384 (comment). I figured it worthwhile to make a PR in case you find them useful.

I might be missing other tests that could get a similar treatment, let me know if that's the case!

@danielcranston danielcranston force-pushed the add_e_notation_test_coverage branch from f4962a8 to e11ae22 Compare February 1, 2025 11:34
@christophebedard christophebedard self-requested a review February 3, 2025 18:04
Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Looks good to me.

This is completely separate from ros2/launch_ros#384 and ros2/launch_ros#436, right? As in, the behaviour this is testing is correct?

@christophebedard
Copy link
Member

Pulls: #824
Gist: https://gist.githubusercontent.com/christophebedard/3a387055330bc86bf34c1c11a029c230/raw/3eefe972d23c9cd96fbc4243f027e4f8ba4e4d3d/ros2.repos
BUILD args: --packages-up-to launch
TEST args: --packages-select launch
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15115

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@danielcranston
Copy link
Contributor Author

Yes I would say it is separate.

As in, the behaviour this is testing is correct?

If "correct" means "consistent with PYYAML 1.1", then yes I believe it's largely correct. I'm not an expert in the nuances of YAML specs, but from the discussion in ros2/launch_ros#384 (comment), I'm a bit on the fence on the _permute_assertion checks though.

If _permute_assertion intends to reflect YAML 1.1 / PYYAML, then there's a small inconsistency:

    _permute_assertion('1234e1', '12340', lc, 'true')  # Arguably wrong. PYYAML treats LHS as a string, so shouldn't equal RHS
    _permute_assertion('1234E2', '123400', lc, 'true')  # Arguably wrong. PYYAML treats LHS as a string, so shouldn't equal RHS
    _permute_assertion('1234E2', '1234E2', lc, 'true')  # OK, PYYAML treats the LHS as a string anyway
    _permute_assertion("'1234E2'", '123400', lc, 'false')  # OK, LHS is coerced to string
    _permute_assertion("'1234E2'", '1234E2', lc, 'false')  # OK, LHS is coerced to string
    _permute_assertion("'1234E2'", "'1234E2'", lc, 'true')  # OK I guess.

Though I have a feeling that, for original authors, the measure of "correctness" isn't YAML 1.1 but something else (maybe some heuristics to reflect how ROS 1 roslaunch behaved?). I should also mention that it's not yet clear to me what piece of code makes '1234e1' become equal to '12340'.

It's also interesting that '1234E1' == '12340' and '1234E1' == '1234E1' are simultaneously true.

Copy link

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya fujitatomoya merged commit a9258c3 into ros2:rolling Feb 5, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants