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 integration tests to Case Management SDK #16

Merged
merged 3 commits into from
May 19, 2020
Merged

Conversation

huydoan2
Copy link

PR summary

Fixes: <! -- link to issue -->

PR Checklist

Please make sure that your PR fulfills the following requirements:

  • The commit message follows the Angular Commit Message Guidelines.
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • New tests
  • Build/CI related changes
  • Documentation content changes
  • Other (please describe)

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@huydoan2 huydoan2 requested a review from padamstx May 18, 2020 17:11
var offeringType, _ = service.NewOfferingType(casemanagementv1.OfferingType_Group_CrnServiceName, "cloud-object-storage")

var (
caseNumber = "CS1310378"
Copy link

Choose a reason for hiding this comment

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

@huydoan2, I think having a fixed case number makes the integration testing fragile. Tests will fail if this case is permanently closed or customer provided their own credentials for integration testing. Instead of this, I did the following in other repos.

  1. Run a test that creates a new case
  2. Store the newly create case number (maybe a static variable?)
  3. Reuse the new case number in subsequent tests

Copy link
Author

Choose a reason for hiding this comment

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

I remember doing exactly what you said. Not sure why that hard-coded case number is still there. Will fix!

Copy link
Member

Choose a reason for hiding this comment

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

Also, if there are static values that the test needs to use (perhaps an id of a resource that the test depends on but doesn't create itself) we can easily add these values as additional config properties to the case_management.env file and then the test could just retrieve that during its initialization.

Copy link
Author

Choose a reason for hiding this comment

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

We are good. Just fixed the usage of static variable. Thanks, Phil.

Expect(*result.Status).To(Equal("Resolved"))
})

It("Succefully unresolve a case", func() {
Copy link

Choose a reason for hiding this comment

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

Just a heads up. Support org recently submitted a request as below.

  • Customers should not be able to set the resolution code.
  • Resolution codes should be hidden from customers.
  • Only employees can set the resolution code.

We will have to make necessary changes to our API and to this test case.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, Chong. Will modify the test case accordingly to how the API behaves in the future.

@huydoan2 huydoan2 changed the base branch from master to add-cm May 19, 2020 15:27
Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

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

I'll merge this PR into my feature branch, then make a few tweaks before releaseing

@padamstx padamstx merged commit a10b080 into IBM:add-cm May 19, 2020
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.

3 participants