feat: Add graphql coverage analytics type

[suejung-sentry]

Description

Restructure GraphQL schema to move coverage related fields in the Repository type into one level of nesting. This is one batch of fields being refactored in the project.

Cleanup of the duped fields will happen after the frontend is integrated to the new fields (#2282)

BEFORE

type Repository {
  name: String!
  <... other stuff>
  coverage: Float
  coverageSha: String
  hits: Int
  misses: Int
  lines: Int
  measurements(
    interval: MeasurementInterval!
    after: DateTime
    before: DateTime
    branch: String
  ): [Measurement!]!
  <... other stuff>
 }

AFTER

type Repository {
  name: String!
  <... other stuff>
  coverageAnalytics: CoverageAnalytics
 }

type CoverageAnalytics {
  hits: Int # formerly repository.hits
  misses: Int # formerly repository.misses
  lines: Int # formerly repository.lines
  commitSha: String # formerly repository.coverageSha
  percentCovered: Float # formerly repository.coverage
  measurements(
    interval: MeasurementInterval!
    after: DateTime
    before: DateTime
    branch: String
  ): [Measurement!]! # formerly repository.measurements
}

This is the GraphQL query

query CoverageAnalytics($owner:String!, $repo: String!, $interval: MeasurementInterval!) {
    owner(username:$owner) {
      repository(name: $repo) {
        ... on Repository {
          name

          # old
	  hits
          misses
          lines
          coverageSha
          coverage
	  measurements(interval: $interval) {
            timestamp
            avg
            min
            max
          }
  
          # new
          coverageAnalytics {
            hits
            misses
            lines
            commitSha
            percentCovered
            measurements(interval: $interval) {
            	timestamp
          	avg
            	min
            	max
          	}
          }
        }
        ... on ResolverError {
          message
        }
      }
    }
}

Tested in staging by confirming the old fields and new fields return the same data for a given query.

Closes codecov/engineering-team#2280

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.25%. Comparing base (8d7fc48) to head (ceb94e3).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@               Coverage Diff                @@
##               main       #806        +/-   ##
================================================
+ Coverage   96.24000   96.25000   +0.01000     
================================================
  Files           812        814         +2     
  Lines         18602      18656        +54     
================================================
+ Hits          17904      17958        +54     
  Misses          698        698                

Flag	Coverage Δ
unit	92.51% <100.00%> (+0.02%)	⬆️
unit-latest-uploader	92.51% <100.00%> (+0.02%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codecov-staging]

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

[codecov-qa]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
2303	1	2302	6

View the top 1 failed tests by shortest run time

graphql_api.tests.test_repository.TestFetchRepository test_fetch_is_github_rate_limited_but_errors

Stack Traces | 0.378s run time

self = <MagicMock name='warning' id='140588238558304'>
args = ('Error when checking rate limit',)
kwargs = {'extra': {'has_owner': True, 'repo_id': 1874}}
msg = "Expected 'warning' to be called once. Called 2 times.\nCalls: [call('Too many event processors on scope! Clearing lis...processor at 0x7fdd4021cf40>]),\n call('Error when checking rate limit', extra={'repo_id': 1874, 'has_owner': True})]."

    def assert_called_once_with(self, /, *args, **kwargs):
        """assert that the mock was called exactly once and that that call was
        with the specified arguments."""
        if not self.call_count == 1:
            msg = ("Expected '%s' to be called once. Called %s times.%s"
                   % (self._mock_name or 'mock',
                      self.call_count,
                      self._calls_repr()))
>           raise AssertionError(msg)
E           AssertionError: Expected 'warning' to be called once. Called 2 times.
E           Calls: [call('Too many event processors on scope! Clearing list to free up some memory: %r', [<function _make_wsgi_request_event_processor.<locals>.wsgi_request_event_processor at 0x7fdd4021cf40>]),
E            call('Error when checking rate limit', extra={'repo_id': 1874, 'has_owner': True})].

.../local/lib/python3.12/unittest/mock.py:958: AssertionError

During handling of the above exception, another exception occurred:

self = <graphql_api.tests.test_repository.TestFetchRepository testMethod=test_fetch_is_github_rate_limited_but_errors>
mock_log_warning = <MagicMock name='warning' id='140588238558304'>
mock_determine_rate_limit = <MagicMock name='determine_if_entity_is_rate_limited' id='140588082908480'>
mock_determine_redis_key = <MagicMock name='determine_entity_redis_key' id='140588082946912'>

    @patch("shared.rate_limits.determine_entity_redis_key")
    @patch("shared.rate_limits.determine_if_entity_is_rate_limited")
    @patch("logging.Logger.warning")
    @override_settings(IS_ENTERPRISE=True, GUEST_ACCESS=False)
    def test_fetch_is_github_rate_limited_but_errors(
        self,
        mock_log_warning,
        mock_determine_rate_limit,
        mock_determine_redis_key,
    ):
        repo = RepositoryFactory(
            author=self.owner,
            active=True,
            private=True,
            yaml={"component_management": {}},
        )
    
        mock_determine_redis_key.side_effect = Exception("some random error lol")
        mock_determine_rate_limit.return_value = True
    
        data = self.gql_request(
            query_repository
            % """
                isGithubRateLimited
            """,
            owner=self.owner,
            variables={"name": repo.name},
        )
    
        assert data["me"]["owner"]["repository"]["isGithubRateLimited"] is None
    
>       mock_log_warning.assert_called_once_with(
            "Error when checking rate limit",
            extra={
                "repo_id": repo.repoid,
                "has_owner": True,
            },
        )
E       AssertionError: Expected 'warning' to be called once. Called 2 times.
E       Calls: [call('Too many event processors on scope! Clearing list to free up some memory: %r', [<function _make_wsgi_request_event_processor.<locals>.wsgi_request_event_processor at 0x7fdd4021cf40>]),
E        call('Error when checking rate limit', extra={'repo_id': 1874, 'has_owner': True})].

graphql_api/tests/test_repository.py:867: AssertionError

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

[codecov-public-qa]

Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time.

❌ Failed Test Results:

Completed 2309 tests with 1 failed, 2302 passed and 6 skipped.

View the full list of failed tests

pytest

• Class name: graphql_api.tests.test_repository.TestFetchRepository
  Test name: test_fetch_is_github_rate_limited_but_errors
  
  

  self = <MagicMock name='warning' id='140588238558304'>
  args = ('Error when checking rate limit',)
  kwargs = {'extra': {'has_owner': True, 'repo_id': 1874}}
  msg = "Expected 'warning' to be called once. Called 2 times.\nCalls: [call('Too many event processors on scope! Clearing lis...processor at 0x7fdd4021cf40>]),\n call('Error when checking rate limit', extra={'repo_id': 1874, 'has_owner': True})]."
  
      def assert_called_once_with(self, /, *args, **kwargs):
          """assert that the mock was called exactly once and that that call was
          with the specified arguments."""
          if not self.call_count == 1:
              msg = ("Expected '%s' to be called once. Called %s times.%s"
                     % (self._mock_name or 'mock',
                        self.call_count,
                        self._calls_repr()))
  >           raise AssertionError(msg)
  E           AssertionError: Expected 'warning' to be called once. Called 2 times.
  E           Calls: [call('Too many event processors on scope! Clearing list to free up some memory: %r', [<function _make_wsgi_request_event_processor.<locals>.wsgi_request_event_processor at 0x7fdd4021cf40>]),
  E            call('Error when checking rate limit', extra={'repo_id': 1874, 'has_owner': True})].
  
  .../local/lib/python3.12/unittest/mock.py:958: AssertionError
  
  During handling of the above exception, another exception occurred:
  
  self = <graphql_api.tests.test_repository.TestFetchRepository testMethod=test_fetch_is_github_rate_limited_but_errors>
  mock_log_warning = <MagicMock name='warning' id='140588238558304'>
  mock_determine_rate_limit = <MagicMock name='determine_if_entity_is_rate_limited' id='140588082908480'>
  mock_determine_redis_key = <MagicMock name='determine_entity_redis_key' id='140588082946912'>
  
      @patch("shared.rate_limits.determine_entity_redis_key")
      @patch("shared.rate_limits.determine_if_entity_is_rate_limited")
      @patch("logging.Logger.warning")
      @override_settings(IS_ENTERPRISE=True, GUEST_ACCESS=False)
      def test_fetch_is_github_rate_limited_but_errors(
          self,
          mock_log_warning,
          mock_determine_rate_limit,
          mock_determine_redis_key,
      ):
          repo = RepositoryFactory(
              author=self.owner,
              active=True,
              private=True,
              yaml={"component_management": {}},
          )
      
          mock_determine_redis_key.side_effect = Exception("some random error lol")
          mock_determine_rate_limit.return_value = True
      
          data = self.gql_request(
              query_repository
              % """
                  isGithubRateLimited
              """,
              owner=self.owner,
              variables={"name": repo.name},
          )
      
          assert data["me"]["owner"]["repository"]["isGithubRateLimited"] is None
      
  >       mock_log_warning.assert_called_once_with(
              "Error when checking rate limit",
              extra={
                  "repo_id": repo.repoid,
                  "has_owner": True,
              },
          )
  E       AssertionError: Expected 'warning' to be called once. Called 2 times.
  E       Calls: [call('Too many event processors on scope! Clearing list to free up some memory: %r', [<function _make_wsgi_request_event_processor.<locals>.wsgi_request_event_processor at 0x7fdd4021cf40>]),
  E        call('Error when checking rate limit', extra={'repo_id': 1874, 'has_owner': True})].
  
  graphql_api/tests/test_repository.py:867: AssertionError

[suejung-sentry]

I saw warningLog getting called twice and causing the test to fail so loosened the requirement... Made it so it doesn't need to be the only warning log and as long as the rate limit error was logged we're good

[suejung-sentry]

All the tests in this file are basically just copied over from test_repository_measurements.py plus small fixes to reflect the new nesting of the coverageAnalytics type.

[suejung-sentry]

These annotations show up in the graphql playground 🎉

[suejung-sentry]

I propose this pattern of declaring the static type of the "parent" that is the first argument to any ariadne resolver with name of <currentType>Props. Borrows the concept/name from React that indicates it is a collection of strongly-typed stuff passed directly from parent to child. Elsewhere in this repo it wasn't strongly typed and/or was ambiguously named/constructed. It doesn't seem like there's a strong ariadne community convention either.. so we could try this..

The parent (repository) passes this information as below, and the return type (CoverageAnalyticsProps) is clear and discoverable

# example model
  repository: {
     coverageAnalytics: {
        percentCovered: 12
    }
  }

#####

# type declaration for the return value of the "parent" that will be the first (0th) positional argument to any ariadne field resolver of the "child"

class CoverageAnalyticsProps:
    repository: Repository

#####

# repository.coverageAnalytics resolver (see its return type is `CoverageAnalyticsProps`)

@repository_bindable.field("coverageAnalytics")
def resolve_coverage_analytics(repository: Repository, info: GraphQLResolveInfo) -> CoverageAnalyticsProps:
    return CoverageAnalyticsProps(
        repository=repository,
    )

#####

# coverageAnalytics.percentCovered resolver (see its 0th arg type is `CoverageAnalyticsProps`)

@coverage_analytics_bindable.field("percentCovered")
def resolve_percent_covered(parent: CoverageAnalyticsProps, info: GraphQLResolveInfo) -> Optional[float]:
    return parent.repository.recent_coverage if parent else None

One downside (honestly, benefit?) of this approach is we lose the "magic" of Ariadne's inferred/default resolvers where if a field resolver is not explicitly provided, Ariadne tries to infer one by matching the field name with the parent object's attributes. As in if I had passed repository to the child directly instead of CoverageAnalyticsProps.repository, I could have gotten some field matching "for free" without having to write my own resolvers (this is not something I think I want 😆 )

[JerrySentry]

I was thinking there would be a configuration option to disable default resolvers, but for the life of me I can not find any way to do it on the Ariadne docs.

What you proposed sounds like a good alternative to me, though I'm imagining it would take a bit for devs to build the muscle memory to add follow this guideline.

[ajay-sentry]

what would you propose for all our existing patterns? I can see a world where we do this for a few models but never go around to cleaning up the rest.

For that reason, I'd say we should probably look to create a proposal outside of this review and gain consensus / context across the team so anyone who happens to miss this review is still informed with the new pattern going forward, and we have a gameplan and bandwidth allocated to actually do this migration across the board as well.

Off the top of your head, would you expect this to take a single engineer a week, a sprint, etc?

[suejung-sentry]

Thanks so much for your input here and in our Applications Weekly meeting!!
As we discussed, I'll merge this as a trial run of this pattern and we can adjust as we mature team opinions on this.

re: How long to do migrations across the board - uhhh I'd guess it can be on the order of a sprint depending on how familiar the given engineer is already with api, but I'd be more concerned that it's risky, so I'd want to be more sure we want this before touching all those.

Great suggestion there - I spun off this ticket to track some consensus investigation work

[EDIT / update] - It seems like the GraphQL convention is to have the parent take the type of what the graphql resolver would return for that whole parent (and not the db model as we seem to do it in a lot of places). We have the option of going for something like that too.

[suejung-sentry]

Type checks fail because the 500+ line file I touched (repository.py) is missing a bunch of types. I didn't want to touch that side-quest with this right now

[JerrySentry]

All these type annotations and GQL field descriptions are beautiful 💯

[JerrySentry]

I was thinking there would be a configuration option to disable default resolvers, but for the life of me I can not find any way to do it on the Ariadne docs.

What you proposed sounds like a good alternative to me, though I'm imagining it would take a bit for devs to build the muscle memory to add follow this guideline.