Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Implement missing methods in changesetResolver and add ChangesetStats #12753

Merged
merged 10 commits into from
Aug 6, 2020

Conversation

mrnugget
Copy link
Contributor

@mrnugget mrnugget commented Aug 5, 2020

Note to reviewers: sorry about the diff! I split up some files (example: resolvers/changesets.go into resolvers/changeset.go and resolvers/changeset_connection.go), removed old test data and old commented out tests. That makes it a bit hard to review, but I think this structure is much easier to manage in the future.

This PR fixes #12642, #12648 and #12646 by:

  • Reviving the old commented out resolvers.TestCampaigns test, but not in a single test, but more manageable in the form of multiple tests: TestCampaignResolver, TestChangesetResolver, TestChangesetConnectionResolver.
  • Implementing the ChangesetConnectionStats, including repository permissions (see permissions_test.go).
  • Remove all the TODOs and broken stuff in the changesetResolver that was introduced when we said that a changeset can now exist in an unpublished state.

Now we have a much more solid and extensible layer of resolver tests, IMHO, that we can expand in the future. My goal here was not to go for 100% test coverage, but to test 70-80% and leave a structure that allows filling in more test cases (for merged changesets, for Bitbucket Server/Gitlab changesets, etc.)

In order to make the tests less noisy I copied over the helpers I used in service_test.go — is there a way we can de-duplicate those without running into import cycles? cc @LawnGnome

@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #12753 into main will increase coverage by 0.09%.
The diff coverage is 68.16%.

@@            Coverage Diff             @@
##             main   #12753      +/-   ##
==========================================
+ Coverage   50.82%   50.91%   +0.09%     
==========================================
  Files        1439     1440       +1     
  Lines       81094    81229     +135     
  Branches     6606     6552      -54     
==========================================
+ Hits        41212    41361     +149     
+ Misses      36328    36299      -29     
- Partials     3554     3569      +15     
Flag Coverage Δ
#go 52.33% <68.16%> (+0.13%) ⬆️
#integration 24.20% <ø> (ø)
#storybook 12.72% <ø> (ø)
#typescript 47.01% <ø> (ø)
#unit 47.62% <68.16%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/frontend/graphqlbackend/campaigns.go 0.00% <ø> (ø)
enterprise/internal/campaigns/state.go 80.05% <ø> (ø)
internal/campaigns/types.go 27.70% <0.00%> (-0.03%) ⬇️
enterprise/internal/campaigns/resolvers/testing.go 59.18% <59.72%> (+0.51%) ⬆️
...terprise/internal/campaigns/resolvers/changeset.go 50.18% <62.85%> (ø)
...ternal/campaigns/resolvers/changeset_connection.go 76.31% <76.31%> (ø)
...terprise/internal/campaigns/resolvers/campaigns.go 73.87% <0.00%> (-4.51%) ⬇️
...rise/internal/campaigns/resolvers/campaign_spec.go 74.54% <0.00%> (-3.64%) ⬇️
...nterprise/internal/campaigns/resolvers/resolver.go 68.13% <0.00%> (-1.27%) ⬇️
... and 3 more

2 similar comments
@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #12753 into main will increase coverage by 0.09%.
The diff coverage is 68.16%.

@@            Coverage Diff             @@
##             main   #12753      +/-   ##
==========================================
+ Coverage   50.82%   50.91%   +0.09%     
==========================================
  Files        1439     1440       +1     
  Lines       81094    81229     +135     
  Branches     6606     6552      -54     
==========================================
+ Hits        41212    41361     +149     
+ Misses      36328    36299      -29     
- Partials     3554     3569      +15     
Flag Coverage Δ
#go 52.33% <68.16%> (+0.13%) ⬆️
#integration 24.20% <ø> (ø)
#storybook 12.72% <ø> (ø)
#typescript 47.01% <ø> (ø)
#unit 47.62% <68.16%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/frontend/graphqlbackend/campaigns.go 0.00% <ø> (ø)
enterprise/internal/campaigns/state.go 80.05% <ø> (ø)
internal/campaigns/types.go 27.70% <0.00%> (-0.03%) ⬇️
enterprise/internal/campaigns/resolvers/testing.go 59.18% <59.72%> (+0.51%) ⬆️
...terprise/internal/campaigns/resolvers/changeset.go 50.18% <62.85%> (ø)
...ternal/campaigns/resolvers/changeset_connection.go 76.31% <76.31%> (ø)
...terprise/internal/campaigns/resolvers/campaigns.go 73.87% <0.00%> (-4.51%) ⬇️
...rise/internal/campaigns/resolvers/campaign_spec.go 74.54% <0.00%> (-3.64%) ⬇️
...nterprise/internal/campaigns/resolvers/resolver.go 68.13% <0.00%> (-1.27%) ⬇️
... and 3 more

@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #12753 into main will increase coverage by 0.09%.
The diff coverage is 68.16%.

@@            Coverage Diff             @@
##             main   #12753      +/-   ##
==========================================
+ Coverage   50.82%   50.91%   +0.09%     
==========================================
  Files        1439     1440       +1     
  Lines       81094    81229     +135     
  Branches     6606     6552      -54     
==========================================
+ Hits        41212    41361     +149     
+ Misses      36328    36299      -29     
- Partials     3554     3569      +15     
Flag Coverage Δ
#go 52.33% <68.16%> (+0.13%) ⬆️
#integration 24.20% <ø> (ø)
#storybook 12.72% <ø> (ø)
#typescript 47.01% <ø> (ø)
#unit 47.62% <68.16%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/frontend/graphqlbackend/campaigns.go 0.00% <ø> (ø)
enterprise/internal/campaigns/state.go 80.05% <ø> (ø)
internal/campaigns/types.go 27.70% <0.00%> (-0.03%) ⬇️
enterprise/internal/campaigns/resolvers/testing.go 59.18% <59.72%> (+0.51%) ⬆️
...terprise/internal/campaigns/resolvers/changeset.go 50.18% <62.85%> (ø)
...ternal/campaigns/resolvers/changeset_connection.go 76.31% <76.31%> (ø)
...terprise/internal/campaigns/resolvers/campaigns.go 73.87% <0.00%> (-4.51%) ⬇️
...rise/internal/campaigns/resolvers/campaign_spec.go 74.54% <0.00%> (-3.64%) ⬇️
...nterprise/internal/campaigns/resolvers/resolver.go 68.13% <0.00%> (-1.27%) ⬇️
... and 3 more

@mrnugget mrnugget requested a review from a team August 5, 2020 14:38
@mrnugget mrnugget added campaigns batch-changes Issues related to Batch Changes labels Aug 5, 2020
@mrnugget mrnugget added this to the 3.19 milestone Aug 5, 2020
@mrnugget mrnugget marked this pull request as ready for review August 5, 2020 14:38
@mrnugget mrnugget mentioned this pull request Aug 5, 2020
45 tasks
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

You delete code everyday(🎊), when will we have negative lines of code? 🤔

@mrnugget mrnugget merged commit 6a17cca into main Aug 6, 2020
@mrnugget mrnugget deleted the mrn/changesets-resolvers branch August 6, 2020 06:59
@mrnugget
Copy link
Contributor Author

mrnugget commented Aug 6, 2020

@LawnGnome Went ahead and merged this, feel free to leave post-merge feedback!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
batch-changes Issues related to Batch Changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the changesetsConnectionStatsResolver
2 participants