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: Fix test log file name #30654

Closed
wants to merge 1 commit into from
Closed

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 14, 2024

#19385 dropped .cpp infix (see #19385 (comment) and #19385 (comment)). However, src/test/README.md still refers to the foo_tests.cpp.log pattern:

`make check` will write to a log file `foo_tests.cpp.log` and display this file

This PR restores the pre-PR19385 behaviour, as appending is easier to implement than replacing when porting this functionality to CMake.

bitcoin#19385 dropped `.cpp` infix.
However, `src/test/README.md` still refers to the `foo_tests.cpp.log`
pattern.
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 14, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

@fanquake
Copy link
Member

However, src/test/README.md still refer
This PR restores the pre-PR19385 behaviour,

Not sure this PR should be called a "fix"? As it seems the fix would be to just update the docs to match the current (intended) behaviour.

This instead seems to be a reversion to prior behaviour to facilitate something CMake related. I'm curious why this has only become an issue now, given we've already got the tests in CMake? Or is this something that was missing and is being ported now?

@maflcko
Copy link
Member

maflcko commented Aug 14, 2024

Not sure about changing the test-only behavior here. Seems fine to just adjust the readme in the cmake pull, as the line has to be touched anyway. No need to be exact in porting this edge case?

@hebasto
Copy link
Member Author

hebasto commented Aug 14, 2024

This instead seems to be a reversion to prior behaviour to facilitate something CMake related. I'm curious why this has only become an issue now, given we've already got the tests in CMake? Or is this something that was missing and is being ported now?

I had some concerns while documenting CTest behaviour in hebasto#329.

@hebasto hebasto closed this Aug 15, 2024
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.

4 participants