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

Add test for do not prompt when --no-input flag is given #7712

Closed
wants to merge 3 commits into from
Closed

Add test for do not prompt when --no-input flag is given #7712

wants to merge 3 commits into from

Conversation

sk-ip
Copy link
Contributor

@sk-ip sk-ip commented Feb 9, 2020

References #7688

@sk-ip sk-ip requested a review from chrahunt February 10, 2020 15:15
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Hello, thank you for this!

When writing a test like this, we want to make sure that both cases are as close to each other as possible. For that reason, we should first have a test that confirms that we DO prompt for login credentials normally. Then we should reuse almost everything from that test except this time pass --no-input and then confirm that we DO NOT prompt for login credentials.

What I would do for this PR is use the same server setup that you already have but make this a positive test case and check that we actually prompt for login credentials. In a followup PR we can test the negative test case and then officially document and support the --no-input flag.

@sk-ip
Copy link
Contributor Author

sk-ip commented Feb 20, 2020

@chrahunt are there any methods that can tell what is being displayed on the console, I tried to read all the tests, but could not find.

@uranusjr
Copy link
Member

capsys may be helpful.

@pradyunsg
Copy link
Member

a positive test case and check that we actually prompt for login credentials

I think we also want to do this. :)

@sk-ip
Copy link
Contributor Author

sk-ip commented Feb 21, 2020

@pradyunsg actually i was getting some dependency error when i was writing tests, so i rebased it and the issue was resolved :)

Now, i am getting timeout error in running the tests, here is the code

def test_prompt_for_authentication(script):
    server = make_mock_server()
    server.mock.side_effect = [
        authorization_response(),
    ]

    url = "https://{}:{}/simple".format(server.host, server.port)

    args = ["install"]
    args.extend(["--index-url", url])
    args.append("requests")

    with server_running(server):
        result = script.pip(*args, expect_error=True)

    assert "User for localhost:8888: " in result.stdout, str(result)
    assert "Password: " in result.stdout, str(result)

Are there any methods i can use to automatically enter values for the username and password when the tests are being run. i think that's what causing the timeout error, it is requesting for username and password but nothing is being provided during the runtime.

@pradyunsg
Copy link
Member

You can pass stdin="..." to the script.pip call. I'm not 100% sure if that'll work exactly as we want it to -- but it definitely won't hurt to try. :)

        result = script.pip(*args, stdin="user\npass\n", expect_error=True)

@deveshks
Copy link
Contributor

I was taking a look at this PR, and was able to come up with a a positive test case, where I was able to assert "User for localhost:xxxx" in result.stdout after taking inspiration from the first test case.

Will that be enough for testing the positive test case. Is it enough to check that the prompt for username appears in stdout as above? Or do we also want to provide username and password, and verify that the inputted username and password from stdout is what we provided?

@sk-ip, If you are able to move this PR forward and get it merged with the negative test case, I can finish the second test case.

Otherwise I am also happy to take this PR to the finish line myself, and then either create a new PR for the second test case, or use one PR for both.

@sk-ip
Copy link
Contributor Author

sk-ip commented May 1, 2020

@deveshks thanks for this, you may go with adding both the test cases. I almost got lost with this...

@deveshks
Copy link
Contributor

deveshks commented May 1, 2020

@deveshks thanks for this, you may go with adding both the test cases. I almost got lost with this...

Thanks for the response. I will create a new PR adding both the test cases. I think you can also go ahead and close this PR if you want to 😊

@sk-ip sk-ip closed this May 2, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants