Fix Selenium tests; improve reST error reporting

[bjones1]

• Get Selenium tests working on Windows after updates.
• Fix a test failure on Windows/Chrome.
• Include source and line number info in every top-level Runestone node. This improves reST's ability to report line number of errors.

[bnmnetp]

Bryan,

The RunestoneComponent base class already gets source and line. I was thinking this was duplicated effort, but I guess these are all Node descendants.

[bnmnetp]

What was the problem here?

[bjones1]

I see some animation that takes places after manually clicking the "Run" button. Perhaps this needs to finish before the Run button is clickable again? Adding the wait makes this pass on Windows.

======================================================================
ERROR: test_history (runestone.activecode.test.test_activecode.ActiveCodeTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "E:\Runestone\RunestoneComponents\runestone\activecode\test\test_activecode.py", line 42, in test_history
    rb.click()
  File "E:\Downloads\Python36-64\lib\site-packages\selenium\webdriver\remote\webelement.py", line 77, in click
    self._execute(Command.CLICK_ELEMENT)
  File "E:\Downloads\Python36-64\lib\site-packages\selenium\webdriver\remote\webelement.py", line 493, in _execute
    return self._parent.execute(command, params)
  File "E:\Downloads\Python36-64\lib\site-packages\selenium\webdriver\remote\webdriver.py", line 256, in execute
    self.error_handler.check_response(response)
  File "E:\Downloads\Python36-64\lib\site-packages\selenium\webdriver\remote\errorhandler.py", line 194, in check_response
    raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.WebDriverException: Message: unknown error: Element <button class="btn btn-success run-button" disabled="disabled">...</button> is not clickable at point (331, 173). Other element would receive the click: <div class="ac_actions col-md-12">...</div>
  (Session info: chrome=59.0.3071.115)
  (Driver info: chromedriver=2.30.477700 (0057494ad8732195794a7b32078424f92a5fce41),platform=Windows NT 10.0.15063 x86_64)


----------------------------------------------------------------------
Ran 43 tests in 394.236s

FAILED (errors=1)

[bjones1]

Answered questions.

[bjones1]

Exactly -- the nodes need line info, since they're all that remains after the directive is run. (I assume you meant the RunestoneDirective base class in common\runestonedirective.py, not the RunestoneComponent class, which seems implied in the JS?)

[bnmnetp]

Yes, I meant RunestoneDirective Would it be worth creating a RunestoneNode base class so that new components can inherit from it and get the functionality?

[bjones1]

Good point. I think that all Runestone nodes should inherit from a RunestoneNode class. I'm somewhat hesitant to include source and line info in the constructor, just because that doesn't seem to be the pattern in docutils source, and because I'm not as familiar with how to pass par

Option 1: manually set source and line. Inherit from a simple RunestoneNode class.

class RunestoneNode(nodes.Node):
    pass

class ActivcodeNode(nodes.General, nodes.Element, RunestoneNode):
    def __init__(self, content):
        super(ActivcodeNode, self).__init__(name=content['name'])
        self.ac_components = content

class ActiveCode(RunestoneDirective):
    def run(self):
        acnode = ActivcodeNode(self.options)
        acnode.source, acnode.line = self.state_machine.get_source_and_line(self.lineno)

Option 2: Build the source/line info into the base RunestoneNode class. (I'd prefer to use a keyword-only argument, since I'm not sure if some Node subclasses will have other parameters. But that requires Python 3). This seems more trouble than it's worth for the benefit gained.

class RunestoneNode(nodes.Node):
    def __init__(self, directive):
    super(RunestoneNode,self).__init__()
    self.source, self.line = directive.state_machine.get_source_and_line(self.lineno)

class ActivcodeNode(nodes.General, nodes.Element, RunestoneNode):
    def __init__(self, content, directive):
        # Perhaps need to pass self as a kwargs? How to avoid syntax error?
        super(ActivcodeNode, self).__init__(name=content['name'], self)
        self.ac_components = content

class ActiveCode(RunestoneDirective):
    def run(self):
        acnode = ActivcodeNode(self.options, self)

[bnmnetp]

I'm missing something... On option two you are calling:

super(ActivcodeNode, self).__init__(name=content['name'], self)

Why the second self parameter? Thats not how its done in option 1 or in any other code.

[bjones1]

To access self.state_machine.get_source_and_line and to access self.lineno when in the Node class, instead of the Directive class. (I'd assumed your idea of inheritance included automatically setting source and lineno in the superclass.)

[bjones1]

I played a bit, and realized there's no good way to create a RunestoneNode class that works in all places (see below for various scenarios). So, I kept it simple, and manually added code for the differing cases. For example:

# Case 1a: the root node for a directive.
fitbNode = FITBNode(self.options, rawsource=self.block_text)
fitbNode.source, fitbNode.line = self.state_machine.get_source_and_line(self.lineno)

# Case 1b: Using an existing node class.
rawnode = nodes.raw(self.block_text, res, format='html')
rawnode.source, rawnode.line = self.state_machine.get_source_and_line(self.lineno)

# Case 2: created in a role.
blank_node = BlankNode(rawtext)
blank_node.line = lineno

# Case 3: Created as a copy of an existing element.
ffn = FITBFeedbackNode(feedback_field_body.rawsource, *feedback_field_body.children, **feedback_field_body.attributes)

So, this PR should be complete and ready to accept. It passes all tests on my PC.

[bjones1]

A question: in the latest commit, I changed a number of directives that inherited from Directive to inherit instead from RunestoneDirective, assuming this is a good thing. Are there any concerns with this?

[bnmnetp]

Thats good cleanup, to get them all inheriting.

I ran into two problems in my testing, (easy to fix) both tabbedStuff and timedAssessment were missing the **kwargs parameters in their constructor and the call to super.

[bjones1]

Oops! Thanks for the fixes and merge!