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

Acceptance tests #122

Closed
wants to merge 2 commits into from
Closed

Acceptance tests #122

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 27, 2016

Starting with minimal suite that will grow as necessary.

@@ -196,3 +197,36 @@ def test_install_master(self):

def test_install_kra(self):
tasks.install_kra(self.master, first_instance=True)


@pytest.mark.ds_acceptance
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like just simple_replication test, why don't you mark it instead?

Simple replication test even tests if replication is working properly, not just installation.

I would to extend simple_replication to test both cases replication of domain, and replication of ca.

Copy link
Author

Choose a reason for hiding this comment

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

marking TestSimpleReplication: You're right, I don't know why I persuaded myself that it does something else, I'll change that.

extending with ca: No, as was agreed before we don't want to involve CS more that necessary to minimize the risk of failing due to bug other than in DS.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to test if replication between CAs is working



@pytest.mark.cs_acceptance
class TestCSAcceptance(IntegrationTest):
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks for me like just duplication of some installation tests, would be better to mark whole TestInstallWithCA_KRA_DNS1 test?

Copy link
Author

Choose a reason for hiding this comment

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

Are you sure that that marking TestInstallWithCA_KRA_DNS1 would result in:

  • installation of master with dns, ca
  • installation of replica with ca, no dns
  • installation of kra on the master
  • instalation of kra on the second replica
    as was agreed some time ago?

Copy link
Contributor

Choose a reason for hiding this comment

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

test steps:

  • master with ca and kra, no dns
  • replica0 caless, no dns
  • replica0 install ca
  • replica0 install kra
  • replica0 install dns
  • replica1 with CA, no dns
  • replica1 install kra
  • replica1 install dns
  • replica2 install CA + KRA # this is more or less the same as replica 1 because now kra is supported only as two step installation (maybe we can remove this test)
  • replica2 install dns

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe not, we had several bugs found only on third replica

@MartinBasti
Copy link
Contributor

I quite disagree with marks, please read my inline comments

@MartinBasti MartinBasti self-assigned this Sep 27, 2016
@MartinBasti
Copy link
Contributor

Please see my inline comments, I would not bother with removing DNS tests from suite because they are fast and should not affect CA/KRA

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Nov 22, 2016
@martbab martbab added the pushed Pull Request has already been pushed label Nov 22, 2016
@martbab martbab closed this Nov 22, 2016
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
2 participants