Navigation Menu

Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

move/merge test utility funcs in a '/test' and '/test/olm' pkgs #57

Merged
merged 4 commits into from Feb 11, 2020

Conversation

xcoulon
Copy link
Contributor

@xcoulon xcoulon commented Feb 10, 2020

this avoids having multiple packages with test utility funcs,
especially in pkg/test

Signed-off-by: Xavier Coulon xcoulon@redhat.com

this avoids having multiple packages with test utility funcs,
especially in `pkg/test`

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Copy link
Contributor

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

could you please remove the deploy/test/empty.yaml from the PR?
To be honest, I'm not sure if the olm dir containing all test files has really a good name - most of it doesn't relate to OLM. How about a name assertion and move the awaitility.go out of the dir?

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
@xcoulon
Copy link
Contributor Author

xcoulon commented Feb 10, 2020

could you please remove the deploy/test/empty.yaml from the PR?
To be honest, I'm not sure if the olm dir containing all test files has really a good name - most of it doesn't relate to OLM. How about a name assertion and move the awaitility.go out of the dir?

ok, let me try that: renaming test/olm to test/assert (which we may alias to testassert in the import statements) and move awaitility to the test parent package

@xcoulon
Copy link
Contributor Author

xcoulon commented Feb 10, 2020

could you please remove the deploy/test/empty.yaml from the PR?
To be honest, I'm not sure if the olm dir containing all test files has really a good name - most of it doesn't relate to OLM. How about a name assertion and move the awaitility.go out of the dir?

ok, let me try that: renaming test/olm to test/assert (which we may alias to testassert in the import statements) and move awaitility to the test parent package

actually, it's not possible: awaitility.go calls the ConditionMatch func and other assertions use the PollOnceOrUntilCondition (among others) so all funcs need to be in the same pkg. I will go with assert.

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
@codecov
Copy link

codecov bot commented Feb 10, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@73ebd1f). Click here to learn what that means.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##             master      #57   +/-   ##
=========================================
  Coverage          ?   78.86%           
=========================================
  Files             ?        6           
  Lines             ?      407           
  Branches          ?        0           
=========================================
  Hits              ?      321           
  Misses            ?       71           
  Partials          ?       15
Impacted Files Coverage Δ
pkg/installation.go 69.23% <33.33%> (ø)

@xcoulon
Copy link
Contributor Author

xcoulon commented Feb 10, 2020

could you please remove the deploy/test/empty.yaml from the PR?
To be honest, I'm not sure if the olm dir containing all test files has really a good name - most of it doesn't relate to OLM. How about a name assertion and move the awaitility.go out of the dir?

ok, let me try that: renaming test/olm to test/assert (which we may alias to testassert in the import statements) and move awaitility to the test parent package

actually, it's not possible: awaitility.go calls the ConditionMatch func and other assertions use the PollOnceOrUntilCondition (among others) so all funcs need to be in the same pkg. I will go with assert.

done in e975750

Copy link
Contributor

@MatousJobanek MatousJobanek 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

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MatousJobanek, xcoulon

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:
  • OWNERS [MatousJobanek,xcoulon]

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

@xcoulon
Copy link
Contributor Author

xcoulon commented Feb 10, 2020

looks good

cool, thanks for the review and the feeback/suggestions, @MatousJobanek 🙌

@xcoulon xcoulon merged commit c83f963 into codeready-toolchain:master Feb 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants