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

Fix failed tests (partially): Ignore case when letters are created by libraries #22

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

ihcomega56
Copy link
Contributor

@ihcomega56 ihcomega56 commented Dec 6, 2022

Description

This problem is found because regular executions of integration tests failed.
GitHub Actions log: https://github.com/pyrsia/pyrsia-integration-tests/actions/runs/3630701057
Note: There is another failed test that is not fixed by this PR (Investigating & asking the team).

CLI outputs changed like from USAGE to usage, which is caused by Clap crate updates (Seems related to: clap-rs/clap#4123, clap-rs/clap#4155, and pyrsia/pyrsia#1343).
Its codes are outside of Pyrsia and not controllable, so this PR added settings to ignore case.

After Clap update (Causing tests failure)

pyrsia_cli 0.2.1 (c65228f876b76de660660d259ea561a194c555db)
Decentralized Package Network

USAGE:
    pyrsia [SUBCOMMAND]

OPTIONS:
    -h, --help       Print help information
    -V, --version    Print version information

SUBCOMMANDS:
    authorize      Add an authorized node
    build -b       Request a new build
    config -c      Configure Pyrsia
    inspect-log    Show transparency logs
    list -l        Show a list of connected peers
    ping           Pings configured pyrsia node
    status -s      Show information about the Pyrsia node
    help           Print this message or the help of the given subcommand(s)

Before Clap update

Decentralized Package Network

Usage: pyrsia [COMMAND]

Commands:
  authorize    Add an authorized node
  build, -b    Request a new build
  config, -c   Configure Pyrsia
  inspect-log  Show transparency logs
  list, -l     Show a list of connected peers
  ping         Pings configured pyrsia node
  status, -s   Show information about the Pyrsia node
  help         Print this message or the help of the given subcommand(s)

Options:
  -h, --help     Print help information
  -V, --version  Print version information

@ihcomega56 ihcomega56 changed the title Fix failed tests (partially): Ignore case if letters are created by libraries Fix failed tests (partially): Ignore case when letters are created by libraries Dec 6, 2022
@ihcomega56
Copy link
Contributor Author

ihcomega56 commented Dec 7, 2022

@karolh2000 could you review it thought it fixes just one of failures? Another one is under investigation, and it may take some time.

@@ -28,19 +28,21 @@ teardown() {
}

@test "Testing 'pyrsia help' CLI, check if the help is shown." {
# ignore case in this @test
shopt -s nocasematch
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to disable this option once the test is done otherwise it might impact other tests in this file, the best way to do it is the test tear down method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought so too, but it does not seem to effect other tests as I run them after changing their case. That is why I added a comment line 31.
Also bats-core is doing similar without changing the settings back in their code.

FYI ⬇️ The first one passes, and the second one fails when both are in the same file.

@test "example 1" {
   shopt -s nocasematch

   run echo 'aBc'
   assert_output 'ABC'
}

@test "example 2" {
   run echo 'aBc'
   assert_output 'ABC'
}

Copy link
Member

Choose a reason for hiding this comment

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

yes, but lets play safe in this case and turn off this option when the test is done, those kind of bugs are very difficult to debug so no reason no to do it, thanks

Copy link
Contributor Author

@ihcomega56 ihcomega56 Dec 7, 2022

Choose a reason for hiding this comment

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

Okay I agree. That is safer and does not confuse other maintainers. Added it. Thank you for the good feedback.

@ihcomega56 ihcomega56 merged commit f0f581f into pyrsia:main Dec 7, 2022
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