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

Extend integration tests #3050

Merged
merged 9 commits into from Apr 13, 2019
Merged

Conversation

sebwoj
Copy link
Collaborator

@sebwoj sebwoj commented Mar 3, 2019

This commit adds a few new tests to integration tests. Such tests will prevent
issues like #2966. This will increase overall integration test duration, but will
also increase safety.

Signed-off-by: Sebastian Wojciechowski s.wojciechowski89@gmail.com

@sebwoj sebwoj requested a review from a team as a code owner March 3, 2019 21:03
@sebwoj
Copy link
Collaborator Author

sebwoj commented Mar 3, 2019

@bowlofeggs I'm not sure about the direction of this PR. My plan is to add new integration tests like:

  • GET on every release in DB
  • GET on updates query service
  • EDIT on update
  • CREATE update with bug(test integration with bugzilla if possible)
    ans so on.

The list of test cases can be long, so first I would like to know what do you think.

@bowlofeggs
Copy link
Contributor

+1, we definitely want to add a bunch of integration tests. To get write working, we need to set up an Ipsilon server in our CI suite. @puiterwijk told me that there's a way to configure Ipsilon to have a test user that sounded really easy and handy. So if we configure it that way, it should be pretty straightforward to start testing things like creating updates and editing updates. That would be very nice to have!

Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

Looks good so far!

Rather than making one mega pull request, why don't we focus on finishing out the tests in this one, and then add more tests in subsequent PRs? That was they don't become too huge and are easier to review/edit.

elif value == "logout":
assert "Logout Required" in http_response.text
else:
assert value in http_response.text
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of looping over the keys and values, why not assert the keys you expect directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand. Do you think that it will be better to check for keys in http_response.text instead of values?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant more to ask why we don't just expect the keys to be present and just make the above assertions, as opposed to the large if/else block. For example, why not do things like this instead:

if update_info['request']:
    assert update_info['request'] in http_response.text
assert f"stable threshold: {update_info['stable_karma']}" in http_response.text
<etc>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At first, my intention was just to loop over values and check existance(of raw value) in response, but I find out that most of update's fields are rendered different way than just raw value, so I started to adding more and more if/else statements. I will rewrite it.

assert "Logout Required" in http_response.text
else:
assert value in http_response.text
# ffmarkdown...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# ffmarkdown...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At first I wrote the assertions for comments, but I noticed that we sue "markdown" to create links in the middle of comment text. I decided to drop update's comment tests.

else:
assert value in http_response.text
# ffmarkdown...
# for comment_text in expected_comments:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# for comment_text in expected_comments:

assert value in http_response.text
# ffmarkdown...
# for comment_text in expected_comments:
# assert comment_text in http_response.text
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# assert comment_text in http_response.text

@sebwoj sebwoj changed the title [WIP]Extend integration tests Extend integration tests Mar 14, 2019
@sebwoj sebwoj force-pushed the Extend_integration_tests branch 2 times, most recently from 4f5a750 to eec56f9 Compare March 25, 2019 07:43
Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

Just need to keep the 2018 and rebase - I'll take care of that for you.

@@ -1,4 +1,4 @@
# Copyright © 2018 Red Hat, Inc.
# Copyright © 2019 Red Hat, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright © 2019 Red Hat, Inc.
# Copyright © 2018-2019 Red Hat, Inc.

@bowlofeggs bowlofeggs force-pushed the Extend_integration_tests branch 3 times, most recently from 144bbca to 571e7fc Compare April 11, 2019 15:49
This commit adds a few new tests to integration tests. Such tests will prevent
issues like fedora-infra#2966. This will increase overall integration test duration, but will
also increase safety.

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>
Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

Thanks!

@mergify mergify bot merged commit b9bf954 into fedora-infra:develop Apr 13, 2019
@sebwoj sebwoj deleted the Extend_integration_tests branch April 14, 2019 07:06
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.

None yet

2 participants