Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix backend tests on develop #6977

Merged
merged 2 commits into from
Jun 20, 2019
Merged

Fix backend tests on develop #6977

merged 2 commits into from
Jun 20, 2019

Conversation

lilithxxx
Copy link
Contributor

Explanation

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The linter/Karma presubmit checks have passed.
    • These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
  • The PR is made from a branch that's not called "develop".
  • The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
  • The PR follows the style guide.
  • The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
  • The PR is assigned to an appropriate reviewer.
    • If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
    • If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

@lilithxxx lilithxxx requested a review from nithusha21 as a code owner June 20, 2019 09:28
@lilithxxx lilithxxx requested a review from seanlip June 20, 2019 09:29
nithusha21
nithusha21 approved these changes Jun 20, 2019
View reviewed changes
Copy link
Contributor

@nithusha21 nithusha21 left a comment

Choose a reason for hiding this comment

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

The PR is fine. But how did this pass before but fail after merge? Any insights on that please.

@lilithxxx
Copy link
Contributor Author

@nithusha21 its really strange that the tests passed on the previous PR. It really shouldn't as the previous test should fail(it was incorrect). At the moment I cannot understand why this happened. I will update you if I figure it out.

brianrodri
brianrodri suggested changes Jun 20, 2019
View reviewed changes
core/domain/suggestion_registry_test.py
@@ -956,6 +947,14 @@ def test_pre_update_validate_change_question_dict(self):
'skill_id': 'skill_1'
Copy link
Contributor

@brianrodri brianrodri Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Make it more obvious that this change is different, for example:

different_change = copy.copy(expected_suggestion_dict['change'])
del different_change['linked_skill_ids']

suggestion = # ...

# Adding questions will fail because the change is different.
with self.assertRaisesRegexp(...):
  # ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test will fail because the change is same not different. Not sure what you mean though

Copy link
Contributor

@brianrodri brianrodri Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I manually diffed self.suggestion_dict['change'] and the local change here, and your local one is missing linked_skill_ids. Are you saying that this was unintentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as we discussed offline

@codecov
Copy link

codecov bot commented Jun 20, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (develop@995b667). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #6977   +/-   ##
==========================================
  Coverage           ?   95.86%           
==========================================
  Files              ?      371           
  Lines              ?    52091           
  Branches           ?        0           
==========================================
  Hits               ?    49935           
  Misses             ?     2156           
  Partials           ?        0
Impacted Files Coverage Δ
core/domain/suggestion_registry_test.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 995b667...23573d0. Read the comment docs.

@seanlip seanlip merged commit c1c2c00 into oppia:develop Jun 20, 2019
@lilithxxx lilithxxx deleted the fix_backend_tests_on_develop branch June 20, 2019 16:44
nithusha21 pushed a commit that referenced this pull request Jul 1, 2019
@nithusha21
cherry-pick #6977, fix conflicts
919d335
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nithusha21 nithusha21 nithusha21 approved these changes

@brianrodri brianrodri brianrodri approved these changes

@seanlip seanlip Awaiting requested review from seanlip

Assignees

@lilithxxx lilithxxx

Labels
backend important
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lilithxxx @brianrodri @nithusha21 @seanlip @aks681