Skip to content
This repository has been archived by the owner on Aug 17, 2023. It is now read-only.

Test Seeding Report Tray Seeding Summary Values #234

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Mikek16
Copy link

@Mikek16 Mikek16 commented May 3, 2023

Pull Request Description
Closes #207


Licensing Certification

FarmData2 is a Free Cultural Work and all accepted contributions are licensed as described in the LICENSE.md file. This requires that the contributor holds the rights to do so. By submitting this pull request I certify that I satisfy the terms of the Developer Certificate of Origin for its contents.

@Mikek16 Mikek16 marked this pull request as ready for review May 7, 2023 15:11
@Mikek16
Copy link
Author

Mikek16 commented May 7, 2023

@braughtg This feature branch still contains old commits from previous features. Is there a way i can rebase this feature branch with main to remove these commits before closing the pull request?

@braughtg
Copy link
Member

braughtg commented May 7, 2023

This feature branch still contains old commits from previous features. Is there a way i can rebase this feature branch with main to remove these commits before closing the pull request?

This typically happens when your work is based off of another feature branch. When you merge the upstream main into your branch - which it looks like you did - the changes shown in the "Files changed" tab should then just show your new changes. For some reason, GitHub still shows the old commits. This just seems to be a quirk of the way git / GitHub work.

As long as the "Files changed" tab just shows the new changes it will be fine when it is merged intomain because the changes from those other commits are already there.

Copy link
Member

@braughtg braughtg left a comment

Choose a reason for hiding this comment

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

Overall this test looks pretty good. It is not required for the course, but if you would like this to be merged into the project please address the issues noted in the code and mark it as ready for review again. I will leave this open until the end of May.

@@ -0,0 +1,79 @@

describe("Test the seeding report columns by seeding type", () => {
beforeEach(() => {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be an improvement to use a before here instead of a beforeEach so that the report is generated only once, and then split your it below into multiple tests, one for each quantity. That will better isolate the functionality being tested.

@braughtg braughtg marked this pull request as draft May 9, 2023 14:03
@braughtg
Copy link
Member

If or when this is ready, just mark it as "Ready for review" and I'll look it over again in detail.

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

Successfully merging this pull request may close these issues.

Seeding Report: Test Tray Seeding Summary Values
4 participants