Closed bjones1 closed 3 years ago
Thanks for the feedback. Thinking about this a bit, I think this PR should be focused on paying off some technical debt:
selectquestion
testingTechnical debt: We don't have good test coverage of selectquestion
(in and out of timed exam mode). It's complex, high-impact code, in that bugs will cause a huge amount of frustration (students can't take tests, etc.). So, this makes it fairly fragile (changes are difficult). The reasons for this technical debt become apparent when we look at what's required to carry out this testing; see the following text.
Solution: Write a set of tests for it. These tests must (due to the nature of selectquestion
) run on the Runestone server.
Objection: Should we invest in writing new code for the Runestone server? Looking at it, I expect most of the selectquestion
tests will run in Selenium, with just a little checking that the database was updated correctly. I think that we can re-use most of this code when the new BookServer gets up and running.
Objection: The Runestone components rely on a lot of manual timing for current testing; since selectquestion
most load from the database, we have no good way to know when to test. Which leads us to...
Technical debt: The tests which run on the Runestone Components depend on a certain amount of manually-included delays/waits plus hoping things run in the correct order. There's currently no way to know when a component has finished rendering and loading data from local storage/the server.
Solution: Enable each component to produce some sort of notification when it is ready. But this requires...
Technical debt: The design of the components is somewhat haphazard; some components lack an ID at all, some have two (identical!) IDs, some have an ID only in certain states, etc. This lack of uniformity makes finding a component in order to determine if it's ready difficult.
Solution: Fix the weirdness. However, any changes to the components may break selectquestion
. (Circular reference detected.)
I suggest approaching this with a set of focused, small PRs. Each PR should provide a good test of selectquestion
for one specific question type and contain only the changes absolutely necessary to enable these tests. These PRs should first cover all the well-behaved components which don't need fixing before looking at components that do need fixing.
@bnmnetp, what do you think of this? Do you prefer to leave this monster alone, or would a focused, gradual approach work for you?
@bjones1 -- a focused gradual approach always appeals to me. Especially when it results in getting us to a place where we can continue to make progress on bigger things while having the confidence that we are not breaking exams.
@bjones1 I'm confused when you say clickable and dragndrop don't have ids. I can see very clearly in the code that the template does include an id. Every component has to have an id to use as its name in the questions table in the database.
There is a set of components that are used in exams and then there are some that are not. For example you would not use a showEval in an exam as there is nothing to answer it is purely for visualization.
The HTML before JS transformation has an ID, but the JS transformations delete the ID, so the final rendered components lack it. The changes in this PR add the missing ID back in.
Good point re: components in exams and those not in exams. I assume those not in exams should be the first to be modified? Is there a list of which components selectquestion
supports, so I can start with components not in the selectquestion
critical path?
To reduce PR clutter, I'm closing this.
One of my test frustrations was dealing with timing in Selenium: how do we know when a component is actually ready in order to start testing it? To definitively answer this question, this PR adds the
runestone-component-ready
class to a component when it is ready (all data from local storage/server loaded, all HTML rendered). It also provides await_until_ready
method in the Python test suite, then adds this to the relevant test suites.Overall, I think these changes will make our testing approach must more robust; it's been somewhat haphazard with waits / delays stuck in to kludge things together.
However, the changes have been more extensive than I planned, so I wanted to get @bnmnetp's feedback before I go too much farther. Specifically, I've made the following changes as I went through the code:
runestone-component-ready
" class approach. I've fixed it.data-component
attributes for a single component. I changed it to bedata-subcomponent
for everything but the main<ul>
element.runestone-component-ready
class. I removed the duplicate code.div_id + "_container"
, but never referenced the name. I removed the "_container" suffix to keep a consistent naming convention across the components.