Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

MCKIN-9091: Aggregation integration tests #1289

Merged
merged 5 commits into from
Dec 4, 2018

Conversation

jcdyer
Copy link

@jcdyer jcdyer commented Nov 27, 2018

Create aggregation integration tests.

These are moved over from edx/edx-platform#17703, because it was too hard to keep them in sync with the changing platform, and there was no good end plan for merging the code. Here, it can be merged cleanly, and run with regular platform tests.

JIRA tickets: BB-157, BB-163 , MCKIN-9091

Discussions: See references JIRA tickets, and mainline edx-platform PR.

Dependencies: None

Screenshots:

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: None

Testing instructions:

  1. Read over the discussion on the previous PR (edx/edx-platform#17703) and the referenced tickets.
  2. Verify that the tests provide sufficient coverage as described in BB-163.

Author notes and concerns:

Reviewers

  • (OpenCraft internal reviewer's GitHub username goes here)
  • edX reviewer[s] TBD

Settings
N/A

@Metfriet
Copy link

@jcdyer Are you checking for BlockCompletions submitted in various configurations? I can't find it in the code, but maybe I am overlooking something.

@jcdyer jcdyer force-pushed the cliff/agg-integration-tests branch from 7016428 to 09379d9 Compare December 3, 2018 15:24
@jcdyer
Copy link
Author

jcdyer commented Dec 3, 2018

@Metfriet I added some tests to verify the behavior of different configurations.

@Metfriet
Copy link

Metfriet commented Dec 3, 2018

@jcdyer ok it looks good now. I will approve it when circleci is done.

Copy link

@Metfriet Metfriet left a comment

Choose a reason for hiding this comment

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

I accidentally approved just now, but the tests are actually failing, also I don't have write access so you'll need to find someone else to also review this. @jcdyer

@jcdyer
Copy link
Author

jcdyer commented Dec 3, 2018

Oh weird. Those are passing for me locally.

@jcdyer
Copy link
Author

jcdyer commented Dec 3, 2018

That took longer to diagnose than it should. The tests are passing on lms, but not cms. They need to be flagged @skip_unless_lms.

Copy link

@Metfriet Metfriet left a comment

Choose a reason for hiding this comment

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

looks good 👍 @jcdyer

@lgp171188 lgp171188 self-requested a review December 4, 2018 15:18
Copy link

@lgp171188 lgp171188 left a comment

Choose a reason for hiding this comment

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

👍 approving it based on @Metfriet's review

@jcdyer jcdyer merged commit 9357472 into development Dec 4, 2018
@jcdyer jcdyer deleted the cliff/agg-integration-tests branch December 4, 2018 18:37
@wajeeha-khalid
Copy link

@jcdyer can you please provide MCKIN lira ticket number against this work? I need this info to prepare release

@jcdyer
Copy link
Author

jcdyer commented Dec 14, 2018

This did not have a MCKIN ticket, so I've created one at https://edx-wiki.atlassian.net/browse/MCKIN-9091. The only modifications here are adding tests (and fixing one tiny configuration error in LibraryContent module that was caught by the tests).

@wajeeha-khalid wajeeha-khalid changed the title Aggregation integration tests MCKIN-9091: Aggregation integration tests Dec 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants