Skip to content

Test library failed to export after import - #8889

Merged
Shrhawk merged 1 commit into
masterfrom
shr/bug/TNL-2686-library-export-then-import-error
Aug 6, 2015
Merged

Test library failed to export after import#8889
Shrhawk merged 1 commit into
masterfrom
shr/bug/TNL-2686-library-export-then-import-error

Conversation

@Shrhawk

@Shrhawk Shrhawk commented Jul 13, 2015

Copy link
Copy Markdown
Contributor

TNL-2686

Test for Xblock pr#305 where while setting xml attribute with None value throwing TypeError , solve this by adding an inline check if value is of None Type return empty string to save xml attribute

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@adampalay @awais786 this is failing test

@Shrhawk

Shrhawk commented Jul 13, 2015

Copy link
Copy Markdown
Contributor Author

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.

why put this in a try/except?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because we have to clean the temp directory if any Exception occurs

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.

do try/finally then

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i am using the same directory for export below so add the finally block L#468 for export_library_to_xml

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 don't know if it's a risk if the test fails and the extract_dir isn't cleared because we do a git clean before running the tests. If you want to be careful, wrap the whole test in a try/finally.

@Shrhawk
Shrhawk force-pushed the shr/bug/TNL-2686-library-export-then-import-error branch from 408c743 to 18b5900 Compare July 14, 2015 08:06
@Shrhawk

Shrhawk commented Jul 14, 2015

Copy link
Copy Markdown
Contributor Author

@adampalay review it

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.

you don't need a try/except here. If there's no error, then the test passes. If there is an error, the test will automatically not pass.

@Shrhawk
Shrhawk force-pushed the shr/bug/TNL-2686-library-export-then-import-error branch from 18b5900 to b0093a7 Compare July 15, 2015 13:06
@Shrhawk

Shrhawk commented Jul 15, 2015

Copy link
Copy Markdown
Contributor Author

@adampalay

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.

you don't need this line or L469

@Shrhawk
Shrhawk force-pushed the shr/bug/TNL-2686-library-export-then-import-error branch from b0093a7 to c3b0a3f Compare July 16, 2015 07:38
@adampalay

Copy link
Copy Markdown
Contributor

Once the XBlock change is merged to master, and this PR changes the xblock hash to XBlock master, 👍

@Shrhawk
Shrhawk force-pushed the shr/bug/TNL-2686-library-export-then-import-error branch 3 times, most recently from 6def1fb to f62e32e Compare July 30, 2015 10:52
@Shrhawk

Shrhawk commented Jul 30, 2015

Copy link
Copy Markdown
Contributor Author

@adampalay review it.

@adampalay

Copy link
Copy Markdown
Contributor

👍

@Shrhawk

Shrhawk commented Jul 31, 2015

Copy link
Copy Markdown
Contributor Author

@cpennington , @muzaffaryousaf review it.

@Shrhawk

Shrhawk commented Aug 3, 2015

Copy link
Copy Markdown
Contributor Author

@doctoryes kindly review it .

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.

Does this mean that advertised_start should be set to '' everywhere that it's currently set to None? A recent change added this line:

https://github.com/edx/edx-platform/blob/master/cms/djangoapps/contentstore/views/course.py#L790

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes and thanks for the catching this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@doctoryes done the changes have a look

@Shrhawk
Shrhawk force-pushed the shr/bug/TNL-2686-library-export-then-import-error branch from f62e32e to 3dc14c8 Compare August 4, 2015 10:22
@doctoryes

Copy link
Copy Markdown
Contributor

👍

@adampalay

Copy link
Copy Markdown
Contributor

@Shrhawk , you need to merge https://github.com/edx/XBlock/pull/305 and then update the xblock hash here before you merge this PR

@Shrhawk
Shrhawk force-pushed the shr/bug/TNL-2686-library-export-then-import-error branch from 3dc14c8 to 5aaed63 Compare August 5, 2015 18:37
@Shrhawk
Shrhawk force-pushed the shr/bug/TNL-2686-library-export-then-import-error branch from 5aaed63 to 4c1c461 Compare August 5, 2015 19:53
Shrhawk added a commit that referenced this pull request Aug 6, 2015
…n-import-error

Test library failed to export after import
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.

3 participants