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

tests/tail: Add and fix tests. #4727

Closed

Conversation

Joining7943
Copy link
Contributor

This pr also adds tests which would currently fail. They are ignored until fixed.

This pr also fixes some of the tests:

  • Fixes expected newline characters at the end of error messages
  • The error message for directory errors on windows and unix systems actually differ.

@Joining7943 Joining7943 force-pushed the refactor-tail-add-and-fix-tests branch from 79b0e8a to 42831e4 Compare April 10, 2023 17:25
@Joining7943 Joining7943 force-pushed the refactor-tail-add-and-fix-tests branch from 42831e4 to 5637d83 Compare April 10, 2023 17:27
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@Joining7943
Copy link
Contributor Author

The failing tests on android and freebsd are not related to this pr as far as I can see

@Joining7943 Joining7943 mentioned this pull request Apr 10, 2023
@tertsdiepraam
Copy link
Member

tertsdiepraam commented Apr 12, 2023

I think you're still conflating multiple changes. Basically, your original PR contains:

  • Refactor
  • Fixes and corresponding tests
  • Tests that already pass on main

That should be the split here, because introducing these tests that currently fail is not really helpful. It makes more sense to review them as part of the PR that actually fixes them. It's not about just chunking it up based on which code you change, but about the functional changes. Also note that in this PR you refer to another PR that is not merged and might not be. The changes might be moved to another PR or be blocked for some other reason. Of course, we don't want this to happen, but it's possible so it doesn't make sense to refer to the PR.

@Joining7943
Copy link
Contributor Author

Ok, looks like I misunderstood your comment. You said in #3952 something, that you wanted the changes to the tests separated. Personally, I like having tests up front, failing or not, and if you don't like merging my follow up prs you still got the failing tests which show the wrong behavior. However, I'm closing this pr then.

@tertsdiepraam
Copy link
Member

Personally, I like having tests up front, failing or not, and if you don't like merging my follow up prs you still got the failing tests which show the wrong behavior

For development I agree, but as PRs it becomes disjointed. I misspoke in #3952, because I didn't see that there were failing tests in there. Sorry, my mistake :)

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.

2 participants