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

fix: use correct test number sequence for raising github issues #1128

Merged
merged 5 commits into from
Jun 20, 2024

Conversation

stalgiag
Copy link
Contributor

@stalgiag stalgiag commented Jun 10, 2024

This PR modifies createIssueLink() so that it accepts a new property in the options param, testSequenceNumber. In all instances where this function is used, the generated number used for rendering the test number ("Test X:") is passed. This handles confusion about which test is referenced when creating issues. The test row number which is now decoupled from the specific report's sequence is still stored in the hiddenMetadata of the issue.

I wanted to add a unit test but this would be difficult with the current design of this method since the testable logic for this particular problem has more to do with the generated sequence passed in. I can think of ways to adjust the design but none is an absolute win so I decided this was out of scope. Let me know if reviewers think otherwise.

addresses #1110


@stalgiag stalgiag requested review from howard-e and gnarf June 10, 2024 23:45
@stalgiag stalgiag changed the title Fix: use correct test number sequence for raising github issues fix: use correct test number sequence for raising github issues Jun 11, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding some propTypes for verification of params to this.

Also noticed at the bottom there is a "query" still using the old testRowNumber

Copy link
Contributor Author

@stalgiag stalgiag Jun 12, 2024

Choose a reason for hiding this comment

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

Good call! Didn't even notice the query down there.

Addressed in ddb515b and f04ad78 respectively.

Note that we won't get actual param verification since this isn't a React component and we aren't using Typescript but went for the best we can offer for now, JSDoc.

Copy link
Contributor

@gnarf gnarf left a comment

Choose a reason for hiding this comment

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

very simple change needed IMO (the query should be updated to use the correct sequence number instead of row number)

@stalgiag stalgiag requested a review from gnarf June 17, 2024 17:02
Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

@stalgiag left an inline change request. All the other changes look good to me

client/components/TestRun/index.jsx Show resolved Hide resolved
Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for addressing the feedback

@howard-e howard-e merged commit 70270e3 into development Jun 20, 2024
2 checks passed
@howard-e howard-e deleted the correct-issue-num-sequence branch June 20, 2024 19:27
howard-e added a commit that referenced this pull request Jun 24, 2024
Includes the following changes:
* #1123, addresses #791 and #792 with:
  * #1055
  * #1001
  * #1065
  * #1052 
  * #1087
  * #1098 
  * #1092
  * #1131
  * #1124
* #1128, addresses #1100
* #1102, addresses #957
* #1132
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.

3 participants