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.

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

Empty contentpack for distribution and testing #5350

Merged
merged 62 commits into from
Dec 16, 2016

Conversation

benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Dec 1, 2016

Summary

We should finally kill off the English content pack, as the only purpose it serves is to have a function content tree in a blank installation -- for which users should download a full English content pack, anyways.

This will also greatly improve test speeds AFAIK.

TODO

If not all TODOs are marked, this PR is considered WIP (work in progress)

  • Will need to deal with whatever assumptions our tests have been based on by writing a bit of fixtures on top of the empty content db.
  • Make sure the Language tab communicates that it's an empty English language pack.
  • Make sure the Language tab tells users downloading other languages that they MUST install the English language pack.
  • Have tests been written for the new code? If you're fixing a bug, write a regression test (or have a really good reason for not writing one... and I mean really good!)
  • Has documentation been written/updated?
  • New dependencies (if any) added to requirements file
  • After factoring out illegal <a> elements, make sure cursor:pointer is restored Restored because of accessibility concerns, @radinamatic look, you didn't even have to point it out :)
  • Ensure that the test content.db does not get rebuild before every BDD test, just build it once.

Reviewer guidance

Please comment if you have any concerns about testing with no content data :) IMO we need to speed up the tests significantly, so going from 92MB test db to ~0 MB is pretty awesome.

Issues addressed

#5336 #5318

@benjaoming benjaoming added this to the 0.17.0 milestone Dec 1, 2016
@benjaoming benjaoming self-assigned this Dec 1, 2016
@codecov-io
Copy link

codecov-io commented Dec 1, 2016

Current coverage is 51.81% (diff: 78.63%)

Merging #5350 into develop will increase coverage by 0.79%

@@            develop      #5350   diff @@
==========================================
  Files           142        142          
  Lines          7487       7608   +121   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3820       3942   +122   
+ Misses         3667       3666     -1   
  Partials          0          0          

Powered by Codecov. Last update 0969659...b89c622

@benjaoming
Copy link
Contributor Author

All BDD tests are actually working now, other tests are failing because they haven't got a content.db with fixtures.

@benjaoming benjaoming force-pushed the test-contentpack branch 2 times, most recently from a5e7a9b to d1753d9 Compare December 8, 2016 23:42
@benjaoming
Copy link
Contributor Author

Update on English content pack vs. empty content.db:

If there is an empty content.db (i.e. no content packs installed), the user will now be displayed this warning regardless of the situation. For instance, if you have 1 other language, it will still show up. I think this is the intention of all content packs as they do not contain any static exercise data files.

screenshot from 2016-12-11 19-34-50

Benjamin Bach added 21 commits December 12, 2016 01:12
…automatically regenetate when content.db is *probably* a test
@benjaoming benjaoming merged commit 4c7a2b2 into learningequality:develop Dec 16, 2016
@benjaoming benjaoming removed the has PR label Dec 16, 2016
@benjaoming benjaoming changed the title [WIP] Empty contentpack for distribution and testing Empty contentpack for distribution and testing Dec 16, 2016
@benjaoming benjaoming deleted the test-contentpack branch January 17, 2017 16:13
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