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.

Sign up for GitHub

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

Jump to bottom

Improve Cloudflare tests in preparation to fix other issues #1537

Merged
merged 1 commit into from
May 4, 2020
Merged

Improve Cloudflare tests in preparation to fix other issues #1537

merged 1 commit into from
May 4, 2020

Conversation

sheerun
Copy link
Contributor

@sheerun sheerun commented Apr 17, 2020 •
edited
Loading

This PR is first stepping stone to implement fix for #1514. It improves test suite in a way that documents current behavior in tests and will allow to change it in the future. Changes are:

  1. Merge 3 separate mock clients into single, configurable one
  2. Allow to introspect actions performed by provider on client and set expectations on them
  3. Make sure each cloudflare test starts with "TestCloudflare" for easy targeting with -run flag
  4. Improve ApplyChanges test so it shows that indeed Create is performed instead of Update
  5. Disable logrus logging in tests in provider/provider_test.go so it's easier to see test errors
  6. Rewrites existing tests in a way that is agnostic to implementation details (i.e. now tests don't use methods like newCloudFlareChanges)
  7. Adds extra test that shows current end-to-end behavior and bug from Cloudflare provider should use the Update API call #1514

I hope because this PR modifies only tests, it will be quick to review and merge. After that I can start working on fix of #1514 so it will modify much less core and will be easier to review

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 17, 2020
@sheerun
Copy link
Contributor Author

sheerun commented Apr 17, 2020

/assign @Raffo

@sheerun sheerun force-pushed the improved-cloudflare-tests branch 4 times, most recently from 38f9f5d to 2c4c92e Compare April 18, 2020 17:34
@sheerun
Copy link
Contributor Author

sheerun commented Apr 18, 2020

I needed to make one change to make tests deterministic (iterate over sorted keys of map)

@sheerun sheerun force-pushed the improved-cloudflare-tests branch 2 times, most recently from b658b40 to c52028a Compare April 18, 2020 20:36
@sheerun
Copy link
Contributor Author

sheerun commented Apr 18, 2020

Or never mind, I've used DomainFilter to make tests deterministic instead. This is again test-only PR

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 18, 2020
This was referenced Apr 22, 2020
Closed
Merged
Closed
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2020
@sheerun
Copy link
Contributor Author

sheerun commented Apr 24, 2020

done. could someone review this? these are test-only changes

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2020
Raffo
Raffo reviewed Apr 27, 2020
View reviewed changes
go.mod
@@ -33,6 +33,7 @@ require (
github.com/linki/instrumented_http v0.2.0
github.com/linode/linodego v0.3.0
github.com/mattn/go-isatty v0.0.11 // indirect
github.com/maxatome/go-testdeep v1.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes are good, but I do have a question: is there a way to avoid adding a dependency just for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so :( This dependency is very important as all tests are using it. The error messages with it are very clear, and anything else would be either not readable or would require massive amount of code which would be practically duplicating what this dependency already does. Is it big issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Afaik test dependencies don't affect production binary size itself

@Raffo
Copy link
Contributor

Raffo commented May 4, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo, sheerun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit b38ab4a into kubernetes-sigs:master May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Raffo Raffo Raffo left review comments

@TheUltimateC0der TheUltimateC0der TheUltimateC0der approved these changes

@linki linki Awaiting requested review from linki

Assignees

@Raffo Raffo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sheerun @Raffo @k8s-ci-robot @TheUltimateC0der