-
Notifications
You must be signed in to change notification settings - Fork 0
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
Activity Score Automation #462
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #462 +/- ##
==========================================
+ Coverage 91.97% 94.64% +2.67%
==========================================
Files 174 174
Lines 6364 6364
Branches 1349 1349
==========================================
+ Hits 5853 6023 +170
+ Misses 486 316 -170
Partials 25 25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Passing run #1833 ↗︎Details:
Review all test suite changes for PR #462 ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good. I did make a few small suggestions, but you can take them or leave them - your decision. I approve either way.
class ActivityScore { | ||
|
||
verifyActivityScoreSettingsOptionLabel(setting, label) { | ||
const option = ["No score", "Manual", "Rubric", "MCQ Scored"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether this ought to be defined in the class rather than in each method; though it's only used 3 times, so it's debatable.
this.getRubricSummaryDetailsDialog().find('[class^=rubric-table--rubricScoreHeader--]').eq(0).should("contain", "3"); | ||
this.getRubricSummaryDetailsDialog().find('[class^=rubric-table--rubricScoreHeader--]').eq(1).should("contain", "2"); | ||
this.getRubricSummaryDetailsDialog().find('[class^=rubric-table--rubricScoreHeader--]').eq(2).should("contain", "1"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if the use of a method like this would make the above lines more legible:
verifyScoreDisplayInRubricSummaryDetailsDialogHeader(index, containOrNot, string) {
this.getRubricSummaryDetailsDialog().find('[class^=rubric-table--rubricScoreHeader--]').eq(index).should(containOrNot, string);
}
Then each of the lines above would be something like this:
verifyScoreDisplayInRubricSummaryDetailsDialogHeader(0, "not.contain", "3");
(You could also make containOrNot a boolean rather than a string, perhaps.)
It's a close call: my suggestion actually increases the number of lines of code, but makes most of those lines easier to read, I think.
cy.get('[data-cy=feedback-note-toggle-button]').invoke("attr", "title").should("contain", "Note on activity-level feedback"); | ||
cy.get('[data-cy=feedback-settings-toggle]').should("exist"); | ||
cy.get('[data-cy=feedback-settings-toggle]') | ||
.should("contain", "Activity Score:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that some of the values like '[data-cy=activity-level-feedback-button]' and '[data-cy=feedback-settings-toggle]' that are used 5 or 10 times should either be constants or should be put in methods to be called multiple times. The methods could be here in portal-dashboard-activity-score.spec.js, or perhaps in activity-score-settings.js instead.
Activity Score Settings Automation Coverage