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

Test LogReader scenarios for auto_source #32443

Closed

Conversation

markopetkovic96
Copy link

@markopetkovic96 markopetkovic96 commented May 16, 2024

for #31223
no logs available -> should error for all query types: test fail for all query types.
rlogs not available, but qlogs are -> should error with /r, pass with /q and /a: test pass for /q, but fail for /r and /a.
rlogs only available for specific segments, but qlogs are available for the rest -> should error with /r, pass with /a: pass with /r, error with /a.
rlog not available on comma api, but is available from another source (openpilot ci bucket, commaCarSegments), rlogs should be parsed: test return comma_api, where is not available rlog.
I wrote a few tests for auto_source, just to make sure this is a good approach. As you can see, I have deviations from the expected results in several cases. In that case, is it necessary to modify the auto_source function, or is a different approach to the test needed?
For the case where rlog is not available on the comma api, the test just checks the return from auto_source. Can you explain what you mean by rlogs should be parsed? If the return from auto_source is rlog on the comma api, where is not available, rlogs should be parsed?

Copy link
Contributor

github-actions bot commented May 16, 2024

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

@markopetkovic96
Copy link
Author

markopetkovic96 commented May 21, 2024

@jnewb1 After converting the unittests to pytest, I created a mock HTTP server.

Copy link
Contributor

github-actions bot commented Jun 5, 2024

This PR has had no activity for 14 days. It will be automatically closed in 3 days if there is no activity.

@github-actions github-actions bot added the stale label Jun 5, 2024
@markopetkovic96
Copy link
Author

@jnewb1

@github-actions github-actions bot removed the stale label Jun 6, 2024
@adeebshihadeh
Copy link
Contributor

Sorry this took a while to review, but I'm going to close this. It's far too messy: brittle checks (specific error strings), too many lines, wrong indentation, etc.

Feel free to clean this up and resubmit if you'd like (would still honor the bounty).

@markopetkovic96 markopetkovic96 deleted the test_logreader_scenarios branch June 11, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants