Skip to content

run-make tests initialized #7

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

Merged
merged 8 commits into from
Mar 25, 2024
Merged

run-make tests initialized #7

merged 8 commits into from
Mar 25, 2024

Conversation

lordshashank
Copy link

@lordshashank
Copy link
Author

lordshashank commented Mar 23, 2024

@antoyo added libstdc++-12-dev (if this is how you meant), also removed all failing tests so that I can get all the tests failing in ci, independent of my local system, hope thats not an issue.

@antoyo
Copy link
Owner

antoyo commented Mar 23, 2024

It's good. Thanks!

@antoyo
Copy link
Owner

antoyo commented Mar 23, 2024

Weird. We still get the error "/usr/bin/ld: cannot find -lstdc++: No such file or directory". Did you ever get that locally?

@lordshashank
Copy link
Author

lordshashank commented Mar 23, 2024

@antoyo I am thinking to deprecate version to 11, and putting this in ci to debug

    - name: Install packages
      # `llvm-14-tools` is needed to install the `FileCheck` binary which is used for asm tests.
      run: |
          sudo apt-get install ninja-build ripgrep llvm-14-tools libstdc++-11-dev
          locate libstdc++
          echo $LD_LIBRARY_PATH

should I do that (not sure we should do echo in ci runs)?
ALso do review my code, in case there's some mistake, there.

@antoyo
Copy link
Owner

antoyo commented Mar 23, 2024

You want to try version 11 to see if this will fix the tests? If so, yes, this is a good idea.

I wanted to review the PR when the CI pass.
The PR mostly looks good: the changes I would want to see are less code duplication because we'll need to do the same for other test suites in Rust like codegen-units,run-pass-valgrind, and incremental.
So, functions like test_rustc_inner_run_make and the callbacks sent to this function should get refactored to remove code duplication with test_rustc_inner_ui.

Another thing I'm still thinking about is whether we split those new tests like run-make since there seems to be much less of them than UI tests.

@lordshashank
Copy link
Author

@antoyo I tried to reduce duplication by modularising some code (thought a lot, let me know if you have better way). Also I tried to remove the ui tests here to failing-ui-tests, but ci failed so reseted them, but CI still fails. These are running in my device successfully, not getting why are they failing here, could you please help.

@antoyo
Copy link
Owner

antoyo commented Mar 23, 2024

It looks like some test files are being removed twice, causing this error:

Command failed to run: Failed to remove `build/rust/tests/ui/impl-trait/equality-in-canonical-query.rstests/ui/asm/x86_64/issue-96797.rs`: Os { code: 2, kind: NotFound, message: "No such file or directory" }

Since we're removing files, I guess we might need to do a git checkout somewhere between the two runs to restore the files.

@lordshashank
Copy link
Author

@antoyo I am not able to get the issue here as both the runs are failing here, moreover I am not able to replicate this, any views.

@antoyo
Copy link
Owner

antoyo commented Mar 24, 2024

Would you be OK if I push commits to this PR to attempt to fix the issue?

@lordshashank
Copy link
Author

ya sure 😓, there have ben too many commits, should I squash them first?

@antoyo
Copy link
Owner

antoyo commented Mar 24, 2024

Yes please.

@lordshashank
Copy link
Author

lordshashank commented Mar 24, 2024

@antoyo , you can do changes, actually its showing the change in same file in failing-ui-tests.txt , but its irrelevant i guess cause it shows same line added and removed, some ci issue i guess, look forward to what solution is.

@lordshashank
Copy link
Author

could shifting this test to test.rs for ui-tests from failing-ui-tests.txt resolve the issue?, cause its this test only which is giving issue on all failing ci runs (I rechecked them)

@lordshashank
Copy link
Author

lordshashank commented Mar 24, 2024

@antoyo, so checks are successful finally, let me know of any more improvements,
One I wanted your view on was I removed the tests splitting for run-make tests, but now they are getting ran on both runs unnecessarily, should I reinstate splitting or put a check to run run-make test only when current_parts is 0? (I think later one would be good)

Copy link
Owner

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

Very nice work!

@lordshashank
Copy link
Author

@antoyo, so checks are successful finally, let me know of any more improvements, One I wanted your view on was I removed the tests splitting for run-make tests, but now they are getting ran on both runs unnecessarily, should I reinstate splitting or put a check to run run-make test only when current_parts is 0? (I think later one would be good)

@antoyo should I do this?

@antoyo
Copy link
Owner

antoyo commented Mar 24, 2024

@antoyo, so checks are successful finally, let me know of any more improvements, One I wanted your view on was I removed the tests splitting for run-make tests, but now they are getting ran on both runs unnecessarily, should I reinstate splitting or put a check to run run-make test only when current_parts is 0? (I think later one would be good)

@antoyo should I do this?

I'll think about this. The reason why the ui tests are splitted is to make the CI faster, but this doesn't seem necessary for the make-run tests.

You might not know, but there's a summary of failing tests here which shows the number of failing tests for the Failures CI task.
With your PR, the results shown there only show the failures for the make-run tests: perhaps this is because the ui failing tests do not seem to be ran.

I'd like to see how this will render in the summary when those ui failing tests are ran in the CI: if it looks good, perhaps we can just keep the make-run tests the way it is (ran twice).

@lordshashank
Copy link
Author

all tests successful LFG 🚀. @antoyo let me know if some more changes are needed.

@lordshashank lordshashank requested a review from antoyo March 24, 2024 19:00
Copy link
Owner

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

I missed a few things.
After that, we can merge.
Thanks a lot!

Copy link
Owner

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

One more nitpick ;) .

@lordshashank
Copy link
Author

lordshashank commented Mar 24, 2024

Really sorry @antoyo , I don't know why that spacing at last got ommitted even though I tried to add it 😭, this should work now.

@antoyo
Copy link
Owner

antoyo commented Mar 24, 2024

Really sorry @antoyo , I don't know why that spacing at last got ommitted even though I tried to add it 😭, this should work now.

No worries.
I know that some text editor removes the last new line in files. Perhaps your text editor has a setting to configure this behavior?

Copy link
Owner

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

Sorry, I missed those two comments.
After that, we can merge for real :) .

@lordshashank
Copy link
Author

Done and sorry, I should not have removed the comments, would remember for next time.

@lordshashank lordshashank requested a review from antoyo March 25, 2024 13:06
@lordshashank
Copy link
Author

@antoyo , what other test suites of rust do you look forward to implement, I can try them as I guess they would be fairly easy to implement after this modularization.

@antoyo
Copy link
Owner

antoyo commented Mar 25, 2024

I would find the most important to be the standard library (std) tests.

@antoyo antoyo merged commit 2a88451 into antoyo:master Mar 25, 2024
@lordshashank lordshashank deleted the issue-263 branch March 25, 2024 16:44
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.

Run other types of tests like run-make
2 participants