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

Added automation for NTP options test scenarios #2404

Closed
wants to merge 3 commits into from

Conversation

varunmylaraiah
Copy link
Contributor

@varunmylaraiah varunmylaraiah commented Sep 27, 2018

ipatests: add tests for NTP options usage on server, replica, and client

The following tests are added in test_ntp_options.py :: TestNTPoptions
  - test_server_and_client_install_without_option_n
  - test_server_and_client_install_with_option_n
  - test_server_and_client_install_with_multiple_ntp_server
  - test_server_replica_and_client_install_with_ntp_pool_and_ntp_server
  - test_server_and_client_install_with_mixed_options
  - test_two_step_replica_install_using_ntp_options
  - test_two_step_replica_install_without_ntp_options

Details in the ticket: https://pagure.io/freeipa/issue/7719
and https://pagure.io/freeipa/issue/7723

@flo-renaud
Copy link
Contributor

Hi @varunmylaraiah
thanks for the PR. Please fix pylint error:
ipatests/test_integration/test_ntp_replacement.py:5: [W0611(unused-import), ] Unused import pytest

assert "%s" % ntp_pool in cmd.stdout_text
tasks.uninstall_master(self.master)

def test_server_install_with_multiple_ntp_server(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo, s/potion/option

@rcritten
Copy link
Contributor

Did you want to add this to nightly_rawhide.yaml as well?

Do you plan on negative testing as well? Things like:

  • Mixing -N with --ntp-pool or ntp-server
  • If no ntp server is provided on the cli then DNS discovery will be used to try to find one (might be hard/impossible to make generic)
  • Setting both pools and networks (may be pointless but could prevent regressions)

Copy link
Contributor

@flo-renaud flo-renaud left a comment

Choose a reason for hiding this comment

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

Hi @varunmylaraiah
Can you fix the test definition? The small error has crashed PRCI. Thanks

test_suite: test_integration/test_ntp_replacement.py::TestNTPreplacement
template: *ci-master-f28
timeout: 7200
topology: master_1repl_1client
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be topology: *master_1repl_1client

test_suite: test_integration/test_ntp_replacement.py::TestNTPreplacement
template: *ci-master-f28
timeout: 7200
topology: master_1repl_1client
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be topology: *master_1repl_1client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@flo-renaud flo-renaud added WIP Work in progress - not ready yet for review postponed labels Sep 28, 2018
@flo-renaud
Copy link
Contributor

I am setting the postponed label so that this PR does not get picked by PRCI and does not crash the prci process. You can remove the label as soon as the test definition is fixed.

@varunmylaraiah
Copy link
Contributor Author

Did you want to add this to nightly_rawhide.yaml as well?

Do you plan on negative testing as well? Things like:

  • Mixing -N with --ntp-pool or ntp-server
  • If no ntp server is provided on the cli then DNS discovery will be used to try to find one (might be hard/impossible to make generic)
  • Setting both pools and networks (may be pointless but could prevent regressions)

Thank you for the Review,
Yes, I am trying to add negative scenarios along with Replica and client test cases.

@varunmylaraiah
Copy link
Contributor Author

I am setting the postponed label so that this PR does not get picked by PRCI and does not crash the prci process. You can remove the label as soon as the test definition is fixed.

Thanks, Flo, Will fix the issue.

ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
@varunmylaraiah varunmylaraiah force-pushed the ntp_replacement branch 2 times, most recently from 23f5b02 to 6ffab80 Compare October 8, 2018 10:11
@varunmylaraiah varunmylaraiah removed the WIP Work in progress - not ready yet for review label Oct 9, 2018
@varunmylaraiah varunmylaraiah added the re-run Trigger a new run of PR-CI label Oct 9, 2018
@freeipa-pr-ci freeipa-pr-ci removed re-run Trigger a new run of PR-CI labels Oct 9, 2018
@varunmylaraiah varunmylaraiah added the re-run Trigger a new run of PR-CI label Oct 9, 2018
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Oct 9, 2018
@varunmylaraiah varunmylaraiah added the re-run Trigger a new run of PR-CI label Oct 9, 2018
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Oct 9, 2018
@tiran tiran added the needs review Pull Request is waiting for a review label Oct 15, 2018
tiran
tiran previously requested changes Oct 15, 2018
ipatests/prci_definitions/gating.yaml Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_replacement.py Outdated Show resolved Hide resolved
@tiran tiran added the WIP Work in progress - not ready yet for review label Oct 15, 2018
@Tiboris Tiboris removed the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Nov 30, 2018
@varunmylaraiah varunmylaraiah added the re-run Trigger a new run of PR-CI label Dec 3, 2018
@freeipa-pr-ci freeipa-pr-ci removed re-run Trigger a new run of PR-CI labels Dec 3, 2018
Copy link
Member

@Tiboris Tiboris left a comment

Choose a reason for hiding this comment

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

Hi @varunmylaraiah,

What looks very bad to me is long method names (i did not looked at it before so, sorry).
In fact names of methods could be shorter and doc string will tell us what they are meant to do.
In this case, please see also some doc string suggestions.

Thanks for patience!

ipatests/prci_definitions/nightly_master.yaml Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_options.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_options.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_options.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_options.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_options.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_options.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_options.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_options.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ntp_options.py Show resolved Hide resolved
The following tests are added in test_ntp_options.py :: TestNTPoptions
  - test_server_and_client_install_without_option_n
  - test_server_and_client_install_with_option_n
  - test_server_and_client_install_with_multiple_ntp_server
  - test_server_replica_and_client_install_with_ntp_pool_and_ntp_server
  - test_server_and_client_install_with_mixed_options
  - test_two_step_replica_install_using_ntp_options
  - test_two_step_replica_install_without_ntp_options

Details in the ticket: https://pagure.io/freeipa/issue/7719
and https://pagure.io/freeipa/issue/7723

Signed-off-by: Varun Mylaraiah <mvarun@redhat.com>
	Added test_integration/test_ntp_options.py

Signed-off-by: Varun Mylaraiah <mavrun@redhat.com>
            Added test_integration/test_ntp_options.py

Signed-off-by: Varun Mylaraiah <mvarun@redhat.com>
@Tiboris
Copy link
Member

Tiboris commented Dec 3, 2018

Could you please remove Temp commit?

@Tiboris Tiboris added ipa-4-7 and removed WIP Work in progress - not ready yet for review labels Dec 3, 2018
@Tiboris Tiboris added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Dec 3, 2018
@Tiboris
Copy link
Member

Tiboris commented Dec 3, 2018

master:

  • dde2aa4 ipatests: add tests for NTP options usage on server, replica, and client
  • 715d122 nightly_master.yaml Added test_integration/test_ntp_options.py
  • 83487c4 nightly_rawhide.yaml Added test_integration/test_ntp_options.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
7 participants