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

[NOREF] Initial MTO Milestone resolver tests #1548

Open
wants to merge 5 commits into
base: feature/MINT-3175_mto
Choose a base branch
from

Conversation

ClayBenson94
Copy link
Collaborator

@ClayBenson94 ClayBenson94 commented Dec 3, 2024

NOREF

Description

  • Adds in some tests for some existing logic in MTO Milestone resolvers and mutations

Important

This PR is purposefully not exhaustive - I just wanted to get some tests written for this resolver file so we had somewhere to write other tests, and to validate some of the more complex logic (like Common Milestones creating categories upon their instantiation)

How to test this change

  1. Review the tests in pkg/graph/resolvers/mto_milestone_test.go and make sure they all make sense and are readable
  2. Make sure CI/CD tests pass!

PR Author Checklist

  • I have provided a detailed description of the changes in this PR.
  • I have provided clear instructions on how to test the changes in this PR.
  • I have updated tests or written new tests as appropriate in this PR.
  • Updated the Postman Collection if necessary.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • When approving a PR, provide a reason why you're approving it
    • e.g. "Approving because I tested it locally and all functionality works as expected"
    • e.g. "Approving because the change is simple and matches the Figma design"
  • Don't be afraid to leave comments or ask questions, especially if you don't understand why something was done! (This is often a great time to suggest code comments or documentation updates)
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.

@ClayBenson94 ClayBenson94 requested review from a team as code owners December 3, 2024 14:56
@ClayBenson94 ClayBenson94 requested review from StevenWadeOddball, garyjzhao and OddTomBrooks and removed request for a team December 3, 2024 14:56
@ClayBenson94 ClayBenson94 changed the base branch from main to feature/MINT-3175_mto December 3, 2024 14:56
@ClayBenson94 ClayBenson94 removed the request for review from garyjzhao December 3, 2024 14:57
Copy link
Contributor

@StevenWadeOddball StevenWadeOddball left a comment

Choose a reason for hiding this comment

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

Thanks Clay! This looks great! I added some thoughts about other bits that could be tested, but they don't need to block this. Feel free to add or merge as is!

suite.NoError(err)
suite.Len(categories, 2) // 1 new category (Operations) + Uncategorized

operationsCategory, found := lo.Find(categories, func(item *models.MTOCategory) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the resolver is merged in, I might suggest calling MTOCategoriesGetByID to verify the category and subcategory. I do like that this is validating the category creation though!

Also fine to keep this the way it is since you are validating all categories for the MTO!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll see if I can simplify any of this using this new resolver :)

_, foundInternalFunctions := lo.Find(operationsSubCategories, func(item *models.MTOSubcategory) bool {
return item != nil && item.Name == "Internal functions"
})
suite.True(foundInternalFunctions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to validate a case where the subcategory already exists? Or that it will make a new category if you rename a category?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I might add some TODO comments to consider those cases! I didn't do those right away since I wanted to just kickstart these tests with the happy paths, but I agree these would be good test cases!

@ClayBenson94
Copy link
Collaborator Author

Waiting to merge this until #1541 is in, since there'll be a lot of conflicts on this

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

Successfully merging this pull request may close these issues.

2 participants