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

Adds test coverage for commands and project api routes #275

Merged
merged 9 commits into from
Apr 12, 2024

Conversation

omarrida
Copy link
Contributor

@omarrida omarrida commented Apr 8, 2024

Adds the following test cases:

Commands:

  • CreateProjectSnapshotsTest
  • FetchGitHubProjectsTest
  • FetchProjectStatsTest
  • OutputStatsTest
  • SendStatsToSlackTest

API routes:

  • GetProjectsTest
  • ShowProjectTest

Also makes some minor changes:

  • Removes an unreachable return statement in CachedProjectList.
  • Re-keys the issues and pull requests collections in FetchProjectStats to make it easier to assert their values when testing.
  • Changes how we avoid wrapping in the ProjectResource because the previous approach actually removes wrapping from JsonResource affecting all json resources.
  • Implements the getKey() method on the OrgSlack notifiable to allow testing using Notification::fake().

All test cases passing with these changes and linter is green. Happy to make any changes that you see fit in order to get this merged.

- Swap the withoutWrapping call in favor of setting the static property wrap. Calling self::withoutWrapping will remove the data wrapper for ALL json resources, not just the ProjectResource used in the web routes.
@omarrida
Copy link
Contributor Author

omarrida commented Apr 8, 2024

So it looks like the secrets from a forked repo can't be used when running the test action here... would love a hand to see the best way to get this to green.

@driftingly
Copy link
Member

Hi @omarrida we removed Nova from our tests in #276
If you pull in these changes we should be green here 🤞

Thank you!

@omarrida
Copy link
Contributor Author

omarrida commented Apr 9, 2024

Updated the PR with the latest changes from #276 and we are indeed green here!

Thanks so much for making that change to get this PR through!

@omarrida
Copy link
Contributor Author

Hey @driftingly - just as a reminder, I don't have write access so feel free to merge when you're ready :)

Copy link
Member

@driftingly driftingly left a comment

Choose a reason for hiding this comment

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

Thanks @omarrida 🙌

@driftingly driftingly merged commit e86fb6a into tighten:main Apr 12, 2024
4 checks passed
@omarrida
Copy link
Contributor Author

Thanks again for all the help here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants