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

Upgrade concept-fetch dependency version to 71.0.0 #3829

Closed
jonalmeida opened this issue Jan 22, 2021 · 4 comments · Fixed by #3830
Closed

Upgrade concept-fetch dependency version to 71.0.0 #3829

jonalmeida opened this issue Jan 22, 2021 · 4 comments · Fixed by #3830
Assignees

Comments

@jonalmeida
Copy link
Collaborator

jonalmeida commented Jan 22, 2021

Original issue: mozilla-mobile/fenix#17585

There was an API change in the Request class in concept-fetch which causes java class signature errors in a-s which is currently compiled against an older version.

Testing against a newer Android Components version, I'm able to see this error resolved.

┆Issue is synchronized with this Jira Task
┆Sprint: Backlog

@jonalmeida jonalmeida self-assigned this Jan 22, 2021
@jonalmeida jonalmeida linked a pull request Jan 22, 2021 that will close this issue
4 tasks
@rfk rfk closed this as completed in #3830 Jan 25, 2021
@rfk
Copy link
Contributor

rfk commented Jan 25, 2021

Thanks for digging into the details here!

This feels like the sort of thing that should have been detected by a failing test somewhere in the android-components testsuite, but I guess that all the tests there that would have exercised this code and instead mocking out the network, and thus did not trigger the issue. I wonder whether the rusthttp component could grow a small test that would help us catch similar errors in future. I'm imagining something like:

  • Create a mock concept-fetch implementation that returns a known value, rather than hitting the network.
  • Configure the Rust code to use this mock.
  • Call into a Rust function that hits the network, assert that it returns the mocked value.

Do you think that such a test would have caught the issue?

@jonalmeida
Copy link
Collaborator Author

Yes! I definitely think we should write some tests that uncover this before a PR lands. I've filed mozilla-mobile/android-components#9494 for this that we will prioritize at our next triage meeting.

I've also grown rather affectionate towards our bots of late (Mergify and Github Actions) 🤖 . Would it make sense if a bot made a pull request with the latest AC version as our releases are made so that if a method signature fails, the CI build would fail in a visible fashion? Similar to the dependabot PRs that are automatically made in this project.

@rfk
Copy link
Contributor

rfk commented Jan 25, 2021

Would it make sense if a bot made a pull request with the latest AC version as our releases are made so that if a method
signature fails, the CI build would fail in a visible fashion?

This sounds amazing! 👍

@jonalmeida
Copy link
Collaborator Author

I filed this for myself to investigate as well: mozilla-mobile/relbot#52

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 a pull request may close this issue.

2 participants