Skip to content

Make fixtures part of tests#2814

Merged
pierreprinetti merged 1 commit intogophercloud:v1from
shiftstack:fixtures-internal
Oct 17, 2023
Merged

Make fixtures part of tests#2814
pierreprinetti merged 1 commit intogophercloud:v1from
shiftstack:fixtures-internal

Conversation

@mandre
Copy link
Copy Markdown
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
Copy Markdown
Member

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.
@mandre mandre force-pushed the fixtures-internal branch from 77abd8f to c71fc9d Compare October 16, 2023 12:24
@github-actions github-actions bot added semver:major Breaking change and removed semver:major Breaking change labels Oct 16, 2023
@coveralls
Copy link
Copy Markdown

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
Copy Markdown
Member

@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
@pierreprinetti pierreprinetti deleted the fixtures-internal branch October 17, 2023 11:35
@pierreprinetti pierreprinetti added the v1 This PR targets v1 label Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver:major Breaking change v1 This PR targets v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants