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

Make fixtures part of tests #2814

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

mandre
Copy link
Contributor

@mandre mandre commented Oct 16, 2023

By renaming the fixtures to fixtures_test, we mark them as test, and no longer part of the public API.

Some files were left out as they were included in other tests:

$ grep "/testing" -R openstack | cut -d " " -f 2 | sort | uniq
"github.com/gophercloud/gophercloud/openstack/common/extensions/testing"
"github.com/gophercloud/gophercloud/openstack/identity/v3/tokens/testing"
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/floatingips/testing"
"github.com/gophercloud/gophercloud/openstack/networking/v2/networks/testing"
"github.com/gophercloud/gophercloud/openstack/networking/v2/ports/testing"
"github.com/gophercloud/gophercloud/openstack/objectstorage/v1/accounts/testing"
"github.com/gophercloud/gophercloud/openstack/objectstorage/v1/containers/testing"

This should prevent go-apidiff from complaining when modifying fixtures.

This commit mirrors 7f1d075 that merged in master.

@pierreprinetti
Copy link
Contributor

Can you please mention the corresponding commit on the main branch, in the PR description and possibly in the commit itself?

@github-actions github-actions bot added the semver:major Breaking change label Oct 16, 2023
@pierreprinetti pierreprinetti self-assigned this Oct 16, 2023
By renaming the fixtures to fixtures_test, we mark them as test, and no longer part of the public API.

Some files were left out as they were included in other tests:

    $ grep "/testing" -R openstack | cut -d " " -f 2 | sort | uniq
    "github.com/gophercloud/gophercloud/openstack/common/extensions/testing"
    "github.com/gophercloud/gophercloud/openstack/identity/v3/tokens/testing"
    "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/floatingips/testing"
    "github.com/gophercloud/gophercloud/openstack/networking/v2/networks/testing"
    "github.com/gophercloud/gophercloud/openstack/networking/v2/ports/testing"
    "github.com/gophercloud/gophercloud/openstack/objectstorage/v1/accounts/testing"
    "github.com/gophercloud/gophercloud/openstack/objectstorage/v1/containers/testing"

This should prevent go-apidiff from complaining when modifying fixtures.

This commit mirrors 7f1d075 that merged
in master.
@github-actions github-actions bot added semver:major Breaking change and removed semver:major Breaking change labels Oct 16, 2023
@coveralls
Copy link

coveralls commented Oct 16, 2023

Coverage Status

coverage: 77.421%. remained the same when pulling c71fc9d on shiftstack:fixtures-internal into 3f854fa on gophercloud:v1.

Copy link
Contributor

@pierreprinetti pierreprinetti left a comment

Choose a reason for hiding this comment

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

This is technically a major change we're pushing to the stable branch.

The reason for this is: we're removing most test fixtures from the public API surface. Many changes that should have been "patch" or "minor" fell into the "major" bucket because they changed the test fixtures while adding coverage; and this is bad.

I expect disruption from this change. However, I believe that is necessary and that test code shouldn't have been part of the public API in the first place. Sorry about that.

@pierreprinetti pierreprinetti merged commit 105f33c into gophercloud:v1 Oct 17, 2023
143 of 148 checks passed
@pierreprinetti pierreprinetti deleted the fixtures-internal branch October 17, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants