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

uniq: pass remaining GNU tests #5994

Merged
merged 7 commits into from
Feb 25, 2024
Merged

uniq: pass remaining GNU tests #5994

merged 7 commits into from
Feb 25, 2024

Conversation

zhitkoff
Copy link
Contributor

@zhitkoff zhitkoff commented Feb 21, 2024

This PR makes the following changes in order to pass GNU tests/uniq/uniq.pl:

  1. Rework of how obsolete options are handled - following the same approach as was done for split. Unfortunately it is not possible to implement it with just Clap for all required test cases
  2. In support of (1) above - introduces a module posix with 3 constants and a single function to return POSIX version based on _POSIX2_VERSION environment variable. This module would be needed to add similar functionality to sort, tail and touch later on (most likely to finalize their GNU tests compliance as well)
  3. Rework of how input is processed - switching from requiring UTF8 (and failing if input has invalid UTF8 characters) to processing raw u8 bytes sequence. This is required to pass GNU schar test case and in general aligns the behavior with GNU version
  4. Override some Clap errors to match the exact wording of error messages hardcoded into GNU tests
  5. More tests to cover the changes

Should also fix #3509

Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/uniq/uniq is no longer failing!

@zhitkoff
Copy link
Contributor Author

@sylvestre checking in to see if you have any other comments / suggestions on this PR

//!
use std::env;

pub const OBSOLETE: usize = 199209;
Copy link
Contributor

Choose a reason for hiding this comment

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

what about using an enum here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, initially i tried to implement it as an enum, but discarded in favor of these constants - unfortunately it is inconsistently checked in GNU, so was easier to implement this way for now. Might need to give it another try later on as these will be needed to fully cover GNU tests for at least 2 more utilities

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, thanks

@sylvestre
Copy link
Contributor

Impressive work!

@sylvestre sylvestre merged commit 17174ab into uutils:main Feb 25, 2024
62 checks passed
@zhitkoff zhitkoff deleted the uniq-obs branch February 27, 2024 19:21
ysthakur pushed a commit to ysthakur/coreutils that referenced this pull request Feb 27, 2024
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.

test_uniq::gnu_tests often fails with failed to write to stdin of child: Broken pipe
2 participants