Skip to content

Test coverage improvements - #6499

Merged
antoviaque merged 9 commits into
openedx:content-librariesfrom
open-craft:content_libraries/coverage-increase
Jan 9, 2015
Merged

Test coverage improvements#6499
antoviaque merged 9 commits into
openedx:content-librariesfrom
open-craft:content_libraries/coverage-increase

Conversation

@e-kolpakov

Copy link
Copy Markdown
Contributor

Background: This PR contains the LibraryContent XBlock, which allows to display library content in a course.
JIRA ticket: Content Libraries MVP
Discussions: Architecture discussed extensively on the wiki and in meetings, then the revised proposal was presented to the Arch Council on Oct. 21 and given thumbs up.
Dependencies: none
Partner information: 3rd party-hosted open edX instance, for an edX solutions client.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @e-kolpakov! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?number=6499&repo=edx%2Fedx-platform

@e-kolpakov
e-kolpakov force-pushed the content_libraries/coverage-increase branch from 357cf5d to 48c25b3 Compare January 8, 2015 17:33
@e-kolpakov
e-kolpakov force-pushed the content_libraries/coverage-increase branch from 48c25b3 to 973bde1 Compare January 8, 2015 17:50
@e-kolpakov

Copy link
Copy Markdown
Contributor Author

Last build already have failed cms-acceptance test, which haven't failed in previous build + last commit contained insigificant whitespace changes to make pylint happy, so it's definitely a flaky test.

Previous build: https://jenkins.testeng.edx.org/job/edx-all-tests-auto-pr/21194/
Current build: https://jenkins.testeng.edx.org/job/edx-all-tests-auto-pr/21197/

So, that CMS.Textbooks probably should be included to flaky tests list.

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.

@e-kolpakov

Nit: I would prefer to name this new base class LibraryContentTest since I just had to make the same change in https://github.com/edx/edx-platform/pull/6492/files#diff-6 and some of the new tests don't have much to do with the "container" nature of LibraryContent block.

I would also prefer that in the other new test class below we use the name "LibraryContent" rather than "LibraryContainer" since content is the name of the block that we're testing, and "container" is a different (but related) concept in Studio.

@antoviaque

Copy link
Copy Markdown
Contributor

@cahrens @chrisndodge Would you mind reviewing this PR? It's the result of @e-kolpakov pass on improving the coverage - it doesn't alter functionality so it should be a straightforward review.

The failing build is due to a single flaky bok-choy test, as noted by Eugny above.

@bradenmacdonald bradenmacdonald mentioned this pull request Jan 8, 2015
10 tasks
@bradenmacdonald

Copy link
Copy Markdown
Contributor

Build is passing now :)

@cahrens

cahrens commented Jan 9, 2015

Copy link
Copy Markdown

Since these are only test additions/changes, I am comfortable with it only being reviewed by the Solutions team. Thank you for adding additional coverage!

@antoviaque

Copy link
Copy Markdown
Contributor

@cahrens Great, thank you!

@chrisndodge

Copy link
Copy Markdown
Contributor

@antoviaque @e-kolpakov I went through this. It's a bit out of my domain expertise and I don't think I have any feedback to this and I don't think anyone else on the Solutions Team would have the background as well. Like @cahrens I don't see risk in merging in additional tests (always a good thing). It'd be fine merge in as-is or if you want another opinion, I'd suggest @dmitchell.

EDIT: Plus I see this is a merge to another feature branch and not master.

@antoviaque

Copy link
Copy Markdown
Contributor

@chrisndodge Yes, it just adds additional tests so this shouldn't be able to break anything, I'll merge. Thank you for having gone through this!

antoviaque added a commit that referenced this pull request Jan 9, 2015
@antoviaque
antoviaque merged commit a622c71 into openedx:content-libraries Jan 9, 2015
@e-kolpakov
e-kolpakov deleted the content_libraries/coverage-increase branch June 24, 2015 09:52
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.

6 participants