Skip to content

Add unit tests for instructor tool - #50

Merged
itsjeyd merged 2 commits into
masterfrom
instructor-tool-unit-tests
Aug 10, 2015
Merged

Add unit tests for instructor tool#50
itsjeyd merged 2 commits into
masterfrom
instructor-tool-unit-tests

Conversation

@itsjeyd

@itsjeyd itsjeyd commented Jul 22, 2015

Copy link
Copy Markdown
Member

This PR aims to extend unit test coverage for problem-builder by introducing unit tests for the Instructor Tool block.

It follows up on #47.

@itsjeyd

itsjeyd commented Jul 22, 2015

Copy link
Copy Markdown
Member Author

@e-kolpakov Could you please have a look at what I have so far? With the unit test introduced by 1f603db I'm verifying that individual questions are represented correctly in the root block dropdown. The basic idea is to check whether the Fragment returned by student_view contains a line like this for each question that it's supposed to display:

<option value="block_id" ...>... block_name ...</option>

... where block_name corresponds to the value of the block attribute that is preferred for display purposes. (For example, if the value of block.question is non-empty, we always want to display that instead of block.name, block.display_name, or block_id).

Maybe there's a simpler approach? I haven't worked with the mock library a lot, so I'm probably not using it to its full potential.

CC @antoviaque

@itsjeyd
itsjeyd force-pushed the instructor-tool-unit-tests branch from a909afa to 1f603db Compare July 22, 2015 22:10
@e-kolpakov

Copy link
Copy Markdown
Contributor

@itsjeyd I believe the test tests too much:

  • student_view method builds correct render context
  • template engine uses the context to render expected select options.

I can see two ways to improve (and potentially simplify) it:

  1. Convert it to integration test, than use selenium stuff to check for option values/texts
  2. Patch loader.render_template and make sure it get's the correct rendering context.

Since we have an idea to have less integration and more unit tests I would go for option 2. It lacks testing that template does what it supposed to do, but I don't know any way to test templates except integration tests (if you happen to know one - share the knowledge :))

@itsjeyd

itsjeyd commented Jul 23, 2015

Copy link
Copy Markdown
Member Author

@e-kolpakov Thanks for the feedback! I went for option 2. If you could have a quick look to see if the changes match what you had in mind, that would be great :)

@itsjeyd
itsjeyd force-pushed the instructor-tool-unit-tests branch 2 times, most recently from 33cf296 to bb3c01c Compare August 7, 2015 07:49
@itsjeyd

itsjeyd commented Aug 7, 2015

Copy link
Copy Markdown
Member Author

@e-kolpakov I added six more unit tests and two integration tests addressing issues that came up for #47. Please review. :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@itsjeyd nit: could use a helper method:

def check_block(usage_id, expected_name):
    matching_blocks = [block for block in block_tree if block['id'] == self.block.scope_ids.usage_id]
    self.assertEqual(len(matching_blocks), 1)
    matching_block = matching_blocks[0]
    self.assertEqual(matching_block['name'], expected_name)

@itsjeyd

itsjeyd commented Aug 7, 2015

Copy link
Copy Markdown
Member Author

@e-kolpakov Thanks for the comments! I addressed them all. Please have another look at the code.

@e-kolpakov

Copy link
Copy Markdown
Contributor

@itsjeyd 👍

@itsjeyd itsjeyd changed the title (WIP) Add unit tests for instructor tool Add unit tests for instructor tool Aug 10, 2015
previous PR (#47).

The new tests check if:

- preferred block attrs are used when building block tree for current course
- block tree excludes pb-choice blocks
- blocks are correctly marked as (in)eligible
- new-style keys are handled correctly
- student_view passes calls rendering method with correct template args
- author_view and studio_view show appropriate messages
@itsjeyd
itsjeyd force-pushed the instructor-tool-unit-tests branch from 53f78db to 66e6762 Compare August 10, 2015 09:04
itsjeyd added a commit that referenced this pull request Aug 10, 2015
@itsjeyd
itsjeyd merged commit a9f7743 into master Aug 10, 2015
@xitij2000
xitij2000 deleted the instructor-tool-unit-tests branch December 13, 2019 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants