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

Adding Webhooks selenium tests #2911

Merged
merged 6 commits into from Feb 24, 2022

Conversation

kshinde2512
Copy link
Contributor

@kshinde2512 kshinde2512 commented Feb 22, 2022

Describe your changes :

I worked on the adding selenium tests for Webhooks page

Type of change :

  • Improvement

Checklist:

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my own.
  • I have tagged my reviewers below.
  • I have commented on my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • All new and existing tests passed.

Reviewers

@darth-coder00 @shahsank3t

@github-actions
Copy link

The Java checkstyle failed.

Please run mvn googleformatter:format@reformat-sources in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

public class Webhooks {
WebDriver webDriver;

public Webhooks(WebDriver webDriver) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use lombok RequiredArgsConstructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated lombok @requiredargsconstructor

Events.sendKeys(webDriver, webhooks.getEndpoint(), "test.com");
Events.click(webDriver, webhooks.getSaveWebhook());
WebElement errorMessage = webDriver.findElement(common.errorMessage());
if (errorMessage.isDisplayed()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the test fail if error message is not displayed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is to check if error message is displayed on sending blank endpoint or blank name. Should I surround the error message with try catch block and throw exception if no such element exists?

Copy link
Collaborator

@mithmatt mithmatt Feb 23, 2022

Choose a reason for hiding this comment

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

We expect errorMessage to be displayed.

So the test would be:

if(errorMessae.IsDisplayed()) {
  Assert.assertEquals(errorMessage.getText(), "Webhook event filters are required.");
} else {
  Assert.fail("Expected error message to be displayed")
}

This way it tests that error message is displayed and when it does the message is correct. All other cases it should fail.

In the current implementation it tests if error message is correct only if it is displayed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can simplify this further...

Assert.true(errorMessae.IsDisplayed());
Assert.assertEquals(errorMessage.getText(), "Webhook event filters are required.");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay,I will make the changes

Copy link
Contributor

Choose a reason for hiding this comment

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

@kshinde2512 We should avoid conditions in E2E tests when we are expecting some particular behaviour to occur. This applies for positive as well as negative test cases.

PS: @mithmatt correct me if I have misjudged.

@github-actions
Copy link

The Java checkstyle failed.

Please run mvn googleformatter:format@reformat-sources in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

@sonarcloud
Copy link

sonarcloud bot commented Feb 24, 2022

[catalog] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 29 Code Smells

75.8% 75.8% Coverage
0.0% 0.0% Duplication

Copy link
Collaborator

@mithmatt mithmatt left a comment

Choose a reason for hiding this comment

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

Thanks for adding these tests and incorporating review feedback

@mithmatt mithmatt merged commit 9dd8598 into open-metadata:main Feb 24, 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.

None yet

3 participants