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

Transfer more tests to shared test project #2161

Merged
merged 6 commits into from
Apr 1, 2024
Merged

Conversation

kuhnroyal
Copy link
Member

This PR transfers some download and header/options tests to the shared test project and thus enabled them for the Http2 adapter.

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

@kuhnroyal kuhnroyal added the e: tests Improvements or additions to tests label Mar 25, 2024
@kuhnroyal kuhnroyal self-assigned this Mar 25, 2024
@AlexV525 AlexV525 force-pushed the main branch 3 times, most recently from 4d616d5 to 6ad7313 Compare March 26, 2024 02:22
Copy link
Contributor

Code Coverage Report

Package Base Coverage New Coverage Difference
dio 🟢 80.26% 🟢 81.1% 🟢 0.84%
Overall Coverage 🟢 80.26% 🟢 81.1% 🟢 0.84%

Minimum allowed coverage is 0%, this run produced 81.1%

@kuhnroyal kuhnroyal marked this pull request as ready for review March 27, 2024 22:35
@kuhnroyal kuhnroyal requested a review from a team as a code owner March 27, 2024 22:35
@kuhnroyal kuhnroyal added this pull request to the merge queue Apr 1, 2024
Merged via the queue into cfug:main with commit 1c2843a Apr 1, 2024
3 checks passed
@kuhnroyal kuhnroyal deleted the tests branch April 1, 2024 20:03
@AlexV525
Copy link
Member

AlexV525 commented Apr 3, 2024

https://github.com/cfug/dio/actions/runs/8513054967/job/23315952339#step:15:121

[dio]: ::group::❌ loading test/test_suite_test.dart (failed)
[dio]: Failed to load "test/test_suite_test.dart":
[dio]: ../../../../.pub-cache/git/dio-1c2843a2fab1c3795272df1e81502616761e192e/dio_test/lib/src/test/upload_tests.dart:66:20: Error: No named parameter with the name 'exclusive'.
[dio]:       f.createSync(exclusive: false);
[dio]:                    ^^^^^^^^^
[dio]: ../../../../.pub-cache/git/dio-1c2843a2fab1c3795272df1e81502616761e192e/dio_test/lib/src/test/upload_tests.dart:95:20: Error: No named parameter with the name 'exclusive'.
[dio]:       f.createSync(exclusive: false);
[dio]:                    ^^^^^^^^^

@kuhnroyal
Copy link
Member Author

I think this is happening due to the git dependency.

@kuhnroyal
Copy link
Member Author

This likely happens on the min SDK because we have to use an old melos version there which seems to no create the overrides correctly.

@AlexV525
Copy link
Member

AlexV525 commented Apr 3, 2024

We can also remove the exclusive since it defaults to false.

@kuhnroyal
Copy link
Member Author

Yea but that doesn't really fix the issue, only the current symptom.

Something else is wrong here, I played around in kuhnroyal#28 and the stable workflow failed with analyzer errors that I have fixed in the same PR. Like it is analyzing an older dio version. Not sure.

In any case, the http package is doing using a similar approach and they have their test package in the main pubspec as path dependency: https://github.com/dart-lang/http/blob/master/pkgs/http/pubspec.yaml
I think that's what we need to go back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e: tests Improvements or additions to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants