-
Notifications
You must be signed in to change notification settings - Fork 147
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
run MG tests from new repo #1428
run MG tests from new repo #1428
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1428 +/- ##
==========================================
+ Coverage 17.28% 17.89% +0.60%
==========================================
Files 139 132 -7
Lines 10422 10067 -355
==========================================
Hits 1801 1801
+ Misses 8621 8266 -355
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9094877
to
7cf0cec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor nits, we also an INTEROPERABILITY-TESTS.md, should we modify this in the same PR?
@@ -9,7 +9,7 @@ | |||
"frame_builders": [ | |||
{ | |||
"type": "automatic", | |||
"message_id": "test/message-generator/messages/common_messages.json::setup_connection_job_declarator_with_no_async_flag" | |||
"message_id": "../../test/message-generator/messages/common_messages.json::setup_connection_job_declarator_with_no_async_flag" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to change the relative path... to me it looks like we are still doing the same stuff, only extra step added is cloning the message generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the confusion, but unfortunately there's no easy answer for this.
it all started from the fact that most tests launch roles
via cargo llvm-cov
, which:
- has no proper documentation on why or how it's used
- breaks the paths when we use MG from the new repo
the best solution I could come up with was:
- replace
cargo llvm-cov
withcargo run
- adjust some relative paths on the JSON files
that allowed all tests to continue their execution in the same way as before
6f91825
to
956fd31
Compare
that's a good catch, thanks for pointing it out I rebased the new repo to make sure this file is not removed from the git history there: also rebasing here to make sure we delete it from this repo |
956fd31
to
839241b
Compare
1781478
to
41adec9
Compare
41adec9
to
fa29a20
Compare
b3c7cd8
to
c309678
Compare
c309678
to
6097cc5
Compare
this test has been failing silently (!!!) for a long time
while trying to get a green CI for merging this PR, I found some scary insights
by looking at PRs recenlty merged, we can see in the logs:
the CI proceeds as if the test was successful and we never notice it on this PR, we are removing and then shows up something even worse: the I added a new commit to fix in the end, it's not a big deal since |
part of #1387, more specifically #1387 (comment)
most adaptations consisted of:
llvm-cov
was completely removednote: during #1298 we forgot to delete one MG test, so I'm doing it here