Skip to content

Conversation

mydea
Copy link
Member

@mydea mydea commented Nov 7, 2022

This PR updates the build to only run the "special" CI test jobs when the respective packages have actually changed.

This is done using https://github.com/getsentry/paths-filter with a simple list of directories to watch. We can use the handy shared functionality to avoid duplicating the "global" dependencies.

Note that we do need to keep the internal dependencies in sync (e.g. if nextjs starts to depend on another internal package, we need to add it), but IMHO that is acceptable.

This is how that looks:

Screenshot 2022-11-07 at 15 23 51

Replaces #6067

@mydea mydea added the Dev: CI label Nov 7, 2022
@mydea mydea self-assigned this Nov 7, 2022
@mydea mydea force-pushed the fn/ci-test-streamline branch from 045d464 to 5fb8625 Compare November 7, 2022 14:15
@mydea mydea changed the title chore(ci): Only run ember & nextjs tests when related files changed chore(ci): Only run CI tests when related files changed Nov 7, 2022
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

I like the idea overall. This whole conditional test execution topic heavily reminds me of the academic "test case/suite selection" problem which TLDR boils down to finding a trade-off between fault detection capability (FDC) vs. test execution time. I think though that our particular case is a milder version of the problem as we can specify the dependencies pretty well and we're only making the decision on a package-level. So this doesn't make me too concerned about an overall loss in FDC

However, I think that the "we have to maintain this list" aspect is something we shouldn't underestimate and I'm thinking of two things we could do here:

  • Note that this is a required pre-release step in our SDK release checklist in Notion
  • Create a "package creation" checklist that contains this as a required step (this would probably be handy overall)

WDYT, does this sound reasonable?

We just all need to be aware that this exists and we need to take care of it.

@mydea
Copy link
Member Author

mydea commented Nov 7, 2022

I like the idea overall. This whole conditional test execution topic heavily reminds me of the academic "test case/suite selection" problem which TLDR boils down to finding a trade-off between fault detection capability (FDC) vs. test execution time. I think though that our particular case is a milder version of the problem as we can specify the dependencies pretty well and we're only making the decision on a package-level. So this doesn't make me too concerned about an overall loss in FDC

However, I think that the "we have to maintain this list" aspect is something we shouldn't underestimate and I'm thinking of two things we could do here:

  • Note that this is a required pre-release step in our SDK release checklist in Notion
  • Create a "package creation" checklist that contains this as a required step (this would probably be handy overall)

WDYT, does this sound reasonable?

We just all need to be aware that this exists and we need to take care of it.

Yeah, absolutely true!

I'd also say that in our case it is a fine tradeoff to make, as new packages are added rarely. I think adding it to checklists should be a good step to ensure we don't forget about this! 👍

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

I vote we merge this in once the integrations entry is adjusted. Just had two consecutive Ember test flakes in the morning when releasing.

@mydea mydea force-pushed the fn/ci-test-streamline branch from 814be88 to c6045ef Compare November 8, 2022 13:26
@mydea mydea marked this pull request as ready for review November 8, 2022 15:28
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Just to sanity check - does this mean we don't run these tests on master or on release branches also? We should make sure we are only skipping tests for PRs.

@mydea
Copy link
Member Author

mydea commented Nov 9, 2022

Just to sanity check - does this mean we don't run these tests on master or on release branches also? We should make sure we are only skipping tests for PRs.

Good point!
Regarding release, I will add CHANGELOG.md to the common files, so all tests will always run when that changes - does that sound reasonable? I think that should cover the release branches?

And I'll do some testing to make sure it only applies to PR runs, not any other runs! 👍

@mydea mydea force-pushed the fn/ci-test-streamline branch from c6045ef to d5150b1 Compare November 9, 2022 08:26
@mydea mydea merged commit f1ffb6a into master Nov 9, 2022
@mydea mydea deleted the fn/ci-test-streamline branch November 9, 2022 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants