-
Notifications
You must be signed in to change notification settings - Fork 29
test: Add system tests for exercise administration #3008
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
base: main
Are you sure you want to change the base?
Conversation
e2d0d2a
to
1deabb1
Compare
end | ||
end | ||
|
||
def chosen_select(name, value) |
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.
For now I want to keep the helper methods directly in the test. If further system tests need them, I would consider extracting them.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3008 +/- ##
==========================================
+ Coverage 70.05% 70.20% +0.15%
==========================================
Files 215 215
Lines 6846 6901 +55
==========================================
+ Hits 4796 4845 +49
- Misses 2050 2056 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The exercise administration form is a critical pass. The test will secure the functionality with the upcoming changes. Relates to #2922
1deabb1
to
5b124c0
Compare
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.
look good to me in general, I added a few nit-picky comments and suggestions
within('.markdown-editor__wrapper') do | ||
find('.ProseMirror').set(description) | ||
end |
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 wonder if these very specific class selectors might cause brittleness in the future if a refactoring is upcoming
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 looked at this again. I hoped to find something like role="textbox"
and/or aria-label="Description"
. But I cannot find anything that would be considered expected. I have no practical solution here.
I don't want to make any any UI change in this PR. Eventually I want to make the form accessible to screen readers. That would resolve the issues with testing.
For now I want to use this brittle implementation until we can make this page accessible. 🤔
spec/system/exercises_system_spec.rb
Outdated
|
||
click_button I18n.t('shared.create', model: Exercise.model_name.human) | ||
|
||
expect(page).to have_text 'Exercise has successfully been created.' |
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.
This could also use a translation
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.
Sorry I overlooked this. Fixed!
spec/system/exercises_system_spec.rb
Outdated
let!(:exercise) { create(:fibonacci) } | ||
let(:teacher) { exercise.user } |
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.
let!(:exercise) { create(:fibonacci) } | |
let(:teacher) { exercise.user } | |
let!(:exercise) { create(:fibonacci, user: teacher) } |
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 would move the teacher declaration to the outer scope (and assign the user in the exercise directly)
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.
Right. Thank you! Fixed.
The exercise administration form is a critical pass. The test will secure the functionality with the upcoming changes.
Relates to #2922