Skip to content
This repository was archived by the owner on Jun 7, 2023. It is now read-only.

Conversation

@bjones1
Copy link
Contributor

@bjones1 bjones1 commented Jul 4, 2017

This implements the changes discussed in #405, plus adds a few new features and bug fixes. The final form:

.. fillintheblank:: some_unique_id_here

    Complete the sentence: |blank| had a |blank| lamb. 
    One plus one is: (note that if there aren't enough blanks for the feedback given, 
    they're added to the end of the problem. So, we don't **need** to specify a blank here.)

    -   :Mary: Is the correct answer.
        :Sue: Is wrong.
        :x: Try again. (Note: the last item of feedback matches anything, regardless of the string it's given.)
    -   :little: That's right.
        :.*: Nope.
    -   :2: Right on!
        :2 1: Close.... (The second number is a tolerance, so this matches 1 or 3.)
        :x: Nope. (As earlier, this matches anything.)

Copy link
Member

@bnmnetp bnmnetp left a comment

Choose a reason for hiding this comment

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

Could you update the test source in test/_sources/index.rst to use the new syntax. We are running all of the new tests on our Jenkins server and this one will unless the sources is updated.

# A string of reStructuredText that will be included at the beginning of every
# source file that is read.
rst_prolog = (
# For fill-in-the-blank questions, provide a convenient means to indicate a blank.
Copy link
Member

Choose a reason for hiding this comment

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

Existing books will need to update their conf.py in order to get this. We'll need to do that for pip, thinkcspy, pythonds, etc.

Although if I understand this correctly an author is free to use :blank:foo if they want.

Copy link
Contributor Author

@bjones1 bjones1 Jul 5, 2017

Choose a reason for hiding this comment

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

Yes, that's correct; any form of :blank:`foo` (where foo can be anything) works. Would you like me to submit PRs for these books? Or would you prefer to edit them yourself?

@bjones1
Copy link
Contributor Author

bjones1 commented Jul 5, 2017

Re; test/_sources/index.rst, will do - I'll push an extra commit with this.

@bnmnetp
Copy link
Member

bnmnetp commented Jul 5, 2017

ready to merge this as soon as test/.../index.rst is updated for fitb.

Thanks!

this.feedbackArray = JSON.parse(this.scriptSelector(this.origElem).html());

this.createFITBElement();
this.checkServer("fillb");
Copy link
Member

@bnmnetp bnmnetp Jul 6, 2017

Choose a reason for hiding this comment

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

I'm creating a checklist/writing up a document on writing a component...

You should check for the existence of a sid key in the opts passed to the constructor. If sid is populated (as it will be for grading) then checkServer will do the right thing in the grading interface with respect to populating the correct answer.

In fact the right way to do this is to add an init to RunestoneBase that takes care of this for everything and then make sure that each component calls RunestoneBase.prototype.init.apply(this, arguments)

Note I just wrote this... And I'll push a new runestonebase.js to master momentarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Done in 11d9059.

bjones1 added a commit to bjones1/RunestoneComponents that referenced this pull request Jul 6, 2017
@bjones1
Copy link
Contributor Author

bjones1 commented Jul 6, 2017

I'll work on updating the tests!

@bnmnetp
Copy link
Member

bnmnetp commented Jul 6, 2017

Great shouldn't take you long, you should only need to update index.rst in fitb/test

@bjones1
Copy link
Contributor Author

bjones1 commented Jul 8, 2017

I refactored the test framework to make it cross-platform, and DRY the code. FITB tests pass when using the Chrome driver, but not under PhantomJS -- for some reason, the PhamtomJS driver doesn't transform the HTML on the page into question format (by running code in fitb.js), while Chrome does. ??? I'm not sure if this fails on other platforms or not. I've only tested under Windows.

On Windows/Chrome, I see two test failures. Do these fail on other OSes?

======================================================================
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 41, 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 (334, 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.14393 x86_64)


======================================================================
FAIL: test_dnd4 (runestone.dragndrop.test.test_dragndrop.DragAndDropQuestion_Tests)
Incorrect answer changed to correct
----------------------------------------------------------------------
Traceback (most recent call last):
  File "E:\Runestone\RunestoneComponents\runestone\dragndrop\test\test_dragndrop.py", line 126, in test_dnd4
    self.assertIn("alert-danger", cnamestr)
AssertionError: 'alert-danger' not found in 'alert alert-success draggable-feedback'

----------------------------------------------------------------------
Ran 43 tests in 397.347s

FAILED (failures=1, errors=1)

@bnmnetp
Copy link
Member

bnmnetp commented Jul 9, 2017

If I try to run the tests individually, for example python test_fitb.py from inside the test folder in fitb I get an error:

 from ...unittest_base import module_fixture_maker, RunestoneTestCase
ValueError: attempted relative import beyond top-level package

I'm guessing this is somehow related to your python -m unittest discover but the error seems right to be from ..unittest_base seems like the right path.

@bnmnetp
Copy link
Member

bnmnetp commented Jul 9, 2017

Bryan,

One more thing... We need to keep the Runestone Components compatible with both Python 2.7 and 3.x

Traceback (most recent call last):
  File "/Users/millbr02/anaconda/envs/web2py/lib/python2.7/unittest/loader.py", line 254, in _find_tests
    module = self._get_module_from_name(name)
  File "/Users/millbr02/anaconda/envs/web2py/lib/python2.7/unittest/loader.py", line 232, in _get_module_from_name
    __import__(name)
  File "runestone/__init__.py", line 10, in <module>
    from .fitb import *
  File "runestone/fitb/__init__.py", line 1, in <module>
    from .fitb import *
  File "runestone/fitb/fitb.py", line 207
    str_num, *list_tol = feedback_field_name_raw.split()
             ^
SyntaxError: invalid syntax

@bjones1
Copy link
Contributor Author

bjones1 commented Jul 9, 2017

To run tests on just one question type, python -m unittest discover runestone.fitb from the root directory of RunestoneComponents. (Pytest makes this even easier: pytest -k test_fitb runs the fitb tests only.)

Re: Python 3, oops! I'm spoiled with my shiny new syntax. I'll fix.

@bjones1
Copy link
Contributor Author

bjones1 commented Jul 10, 2017

OK, fixed pushed. On Python 2.7, all tests pass (Chrone driver) except one dnd test.

@bjones1
Copy link
Contributor Author

bjones1 commented Jul 10, 2017

I ran the tests under Python 2.7 and Python 3.6 on upstream/master, and got the same test failures as reported above. Do these fail on Linux as well?

@bnmnetp
Copy link
Member

bnmnetp commented Jul 10, 2017

When I run this on macOS I get 1 fail and 43 errors.

I'm trying to figure out if its a problem with phantoms or what. But its hard to debug because the only way I can get the tests to run AT ALL is to run the top level script. I get import errors when I try to run a single test.

@bnmnetp
Copy link
Member

bnmnetp commented Jul 10, 2017

@bjones1

Changing

from ...unittest_base import x,y,z

to

from runestone.unittest_base import x,y,z 

Allows me to run the script from the local test directory with the line:

python -m unittest test_fitb.py

Once I finally got my old version of chromedriver out of the way all of the fitb tests ran successfully on my mac.

I think we ought to consider the absolute syntax rather than the indirect syntax for the import statements so that we can run the tests individually. This would be the most common mode for development I would think.

@bnmnetp
Copy link
Member

bnmnetp commented Jul 10, 2017

@bjones1 don't change the imports. I've just done it on my local copy of your PR. I'll merge with master and accept if we get same test results. Then we'll see what happens on Jenkins with the CI kicks in.

@bnmnetp bnmnetp merged commit c0b4106 into RunestoneInteractive:master Jul 10, 2017
@bjones1
Copy link
Contributor Author

bjones1 commented Jul 11, 2017

Thanks for the merge. However, I'm certainly in favor of making testing easier -- I'd be very happy to use absolute imports.

@bnmnetp
Copy link
Member

bnmnetp commented Jul 11, 2017

Good, since I made the change 😄

In other news, the failure and the error were both problems. One was where I had mis-placed a close div tag in the dragndrop directive. A second came from your refactoring where we lost the import of ActionChains in the activecode tests.

With those fixes ALL tests are now passing with the Chrome driver.

Hopefully they all pass with the PhantomJS driver on windows too.

In order to help us actually SEE the problems uncovered I've also added a line so that runestone serve sends its output to runestone.log file. -- we could make this a command line option in the future, but this actually seems like decent default behavior.

Its all pushed so if you have a moment, grab the changes and run all the tests on windows with PhantomJS and let me know how it goes.

@bjones1 bjones1 deleted the fitb branch July 11, 2017 15:00
@bjones1
Copy link
Contributor Author

bjones1 commented Jul 11, 2017

Whoops -- sorry for injecting a bug. I'm glad that the tests caught this, plus another problem. The PhantomJS drivers on Windows still fail the tests, however. I'm not sure what's going on there, but I'll just use Chrome instead.

@bnmnetp
Copy link
Member

bnmnetp commented Jul 11, 2017

No worries. That is what the tests are there for.

It also seems that we can use Chrome in a headless mode. Which would allow us to use it instead of PhantomJS for CI testing.

        options = webdriver.ChromeOptions()
        options.add_argument("headless")
        options.add_argument("window-size=1200x800")
        self.driver = webdriver.Chrome(chrome_options=options)  # good for development.

@bjones1
Copy link
Contributor Author

bjones1 commented Jul 11, 2017

Ah, very nice. I trust Chrome a bit more than PhamtomJS, as well...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants