Skip to content

Fixed test_system looking like a test to nosetests, also fixed a class i... - #156

Merged
lapentab merged 6 commits into
masterfrom
blapenta/fix-test-coverage
Jun 19, 2013
Merged

Fixed test_system looking like a test to nosetests, also fixed a class i...#156
lapentab merged 6 commits into
masterfrom
blapenta/fix-test-coverage

Conversation

@lapentab

@lapentab lapentab commented Jun 13, 2013

Copy link
Copy Markdown
Contributor

...n test_error_module.py that had one class inheriting from another

Bethany LaPenta Bethany LaPenta
Fixed test_system looking like a test to nosetests, also fixed a clas…
85f1386 
…s in test_error_module.py that had one class inheriting from another
@ghost ghost assigned wedaly Jun 13, 2013
@wedaly

wedaly commented Jun 17, 2013

Copy link
Copy Markdown
Contributor

@lapentab The PR looks good. It looks like there are some conflicts with master that you will need to resolve before merging. To do this:

  1. Pull the latest version of master into your local repository using git checkout master; git pull
  2. Checkout this branch git checkout blapenta/fix-test-coverage
  3. Merge the changes from master into your branch git merge master
  4. Push the merge commit to the remote branch using git push

(full details here: http://git-scm.com/book/en/Git-Branching-Basic-Branching-and-Merging)

Also, when you assign someone to a PR, please mention them in the PR description using "@". That's the only way to guarantee they'll be notified.

@nedbat

nedbat commented Jun 17, 2013

Copy link
Copy Markdown
Contributor

@lapentab thanks for taking on this thankless task!

"system_test" doesn't seem like a great name for this function. "test_system" was a noun describing what it returned, but we know we don't want that name. "get_test_system" is more accurate, or "make_test_system".

nedbat
nedbat reviewed Jun 17, 2013
View reviewed changes
Comment thread common/lib/xmodule/xmodule/tests/test_error_module.py Outdated

@nedbat nedbat Jun 17, 2013

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.

Duplicating this setUp function is unfortunate. Can't we move this code somewhere we can share it?

@wedaly wedaly Jun 18, 2013

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.

Two possibilities for this:

  1. Create a base class that both test cases inherit from
  2. Define an external helper function that both test cases call in their setUp() methods.

@lapentab

lapentab commented Jun 18, 2013

Copy link
Copy Markdown
Contributor Author

@nedbat @wedaly Renamed get_test_system, does it look ok now?

@wedaly

wedaly commented Jun 18, 2013

Copy link
Copy Markdown
Contributor

@lapentab The rename to get_test_system looks good and is clearer than before. You still need to merge/rebase to resolve conflicts (the "We can't automatically merge this pull request" message should go away once you do this)

@lapentab
Merge branch 'master' into blapenta/fix-test-coverage
b9ca9a8 
Conflicts:
	AUTHORS
	common/lib/xmodule/xmodule/tests/test_annotatable_module.py
	common/lib/xmodule/xmodule/tests/test_capa_module.py
	common/lib/xmodule/xmodule/tests/test_combined_open_ended.py
	common/lib/xmodule/xmodule/tests/test_conditional.py
	common/lib/xmodule/xmodule/tests/test_html_module.py
	common/lib/xmodule/xmodule/tests/test_progress.py
	common/lib/xmodule/xmodule/tests/test_xml_module.py
	lms/djangoapps/courseware/tests/__init__.py
@lapentab

lapentab commented Jun 18, 2013

Copy link
Copy Markdown
Contributor Author

@wedaly Done, but it still says there are conflicts...

wedaly
wedaly reviewed Jun 18, 2013
View reviewed changes
Comment thread common/lib/xmodule/xmodule/tests/test_combined_open_ended.py Outdated

@wedaly wedaly Jun 18, 2013

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.

I think here you want self.get_test_system to be just self.test_system.

@wedaly

wedaly commented Jun 18, 2013

Copy link
Copy Markdown
Contributor

Just two more things:

  1. See my comments about self.get_test_system. Since this is a variable that points to the test system, it should be a noun. We can name an instance variable test_system without causing a problem, because nosetests won't try to run instance variables created at run-time.

  2. Please address Ned's comment about duplication in the setUp methods.

@lapentab

lapentab commented Jun 19, 2013

Copy link
Copy Markdown
Contributor Author

@wedaly Done

@wedaly

wedaly commented Jun 19, 2013

Copy link
Copy Markdown
Contributor

👍

@wedaly

wedaly commented Jun 19, 2013

Copy link
Copy Markdown
Contributor

(You can merge it when you're ready)

lapentab added a commit that referenced this pull request Jun 19, 2013
@lapentab
Merge pull request #156 from edx/blapenta/fix-test-coverage
24bee17 
Fix test_system looking like a test to nosetest. Fix class inheritance causing double counting of coverage.
@lapentab
lapentab merged commit 24bee17 into master Jun 19, 2013
aboudreault pushed a commit to aboudreault/edx-platform that referenced this pull request Aug 14, 2014
@mattdrayer
Merge pull request openedx#156 from edx-solutions/ziafazal/paging-lis…
ce28239 
…t-operations-groupslist-am

implemented paging on groups list
jbau pushed a commit that referenced this pull request Jan 21, 2015
David Adams
Merge pull request #156 from Stanford-Online/dcadams/analytics_origin
e88e8d8 
Inline Analytics - Origin
idegtiarov pushed a commit to Code-Institute-Org/edx-platform that referenced this pull request Sep 17, 2018
@maximst
Merge pull request openedx#156 from raccoongang/ginkgo-rg-fix-test
0b3ebb3 
Fix unit test. (ginkgo-rg)
prabhanshu pushed a commit to prabhanshu/edx-platform that referenced this pull request Oct 13, 2018
@sdolenc
Disable the autocomplete attribute on the password input field (opene…
d51943f 
…dx#164)

identical to PR openedx#156 

* Disable the autocomplete attribute on the password input element
(cherry picked from commit a4ce11e1560a55e300ed0f0a639e6169f0452077)

* Disable the autocomplete attribute on the password input element
(cherry picked from commit 95c06305921e83c706a4131b688064634a76edf7)

* Disable the autocomplete attribute on the password input element
(cherry picked from commit dab23059d65c48188c0ad999a62a1cb587a249ad)

* Disable the autocomplete attribute on the password input element
(cherry picked from commit 417f55f792570ac874419f1b8af5ca8c2f3cda5b)
CrewS pushed a commit to CrewS/edx-platform-1 that referenced this pull request Dec 28, 2018
@gitfengri
Merge pull request openedx#156 from WeJie/fix-1
9a38793 
Fix openedx#4416 通过修改链接，直接无需购买课程，就可以是对课程进行学习
edx-secure pushed a commit that referenced this pull request Aug 19, 2019
adeelehsan
Merge pull request #156 from edx/aehsan/prod-349/remove_google_api_ke…
76a13fa 
…y_from_github

Removed google api key from github
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
@renovate @renovate-bot
fix(deps): update dependency @reduxjs/toolkit to v1.6.1 (openedx#156)
c6df8cd 
Co-authored-by: Renovate Bot <bot@renovateapp.com>
@matiasua matiasua mentioned this pull request Oct 1, 2023
Merged
@sumodgeorge sumodgeorge mentioned this pull request Oct 1, 2023
Open
@baby636 baby636 mentioned this pull request Oct 2, 2023
Open
@matiasua matiasua mentioned this pull request Oct 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@wedaly wedaly

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lapentab @wedaly @nedbat