Competing Evidence: Fix issues from client QA

[itsjeyd]

• Scroll to top when navigating to next step (or loading page)
• Remove ticks and tick labels from plot axes
• Fix video previews
• Extra apostrophe: https://github.com/edx/edx-platform/pull/10581
• Add footer to Problem Builder and Step Builder (LMS theme)

To test make sure you have problem-builder and imagemodal (https://github.com/Stanford-Online/xblock-image-modal) xblocks installed. Import latest export (2015.6PFnhS.tar.gz) from OC-1020 into a fresh course.

[e-kolpakov]

@mtyaka would be nice to have testing instructions and course export that showcases fixes and new features.

[mtyaka]

@e-kolpakov I updated PR description with instructions on how to test.

[e-kolpakov]

@mtyaka @itsjeyd I'm not sure what (and if) I'm doing wrong, but I can't see that copyright notice anywhere (in LMS). I don't have lms theme applied though. So, under what conditions that copyright should be shown, and where?

[mtyaka]

@e-kolpakov Hmmm... it should look like this:

It should always show up as long as you don't have the apros theme installed.

[e-kolpakov]

@mtyaka Looks like I just haven't noticed it - I'll do another run.

UPD: Right, I had Apros theme configured

[e-kolpakov]

@mtyaka @itsjeyd verified, everything works as advertised. However, there are some possible improvements, so please address other notes (i.e. change implementation or provide argumentation for chosen approach).

[e-kolpakov]

@mtyaka 👍 conditional tests pass

[antoviaque]

@mtyaka It looks like the last commit on this has a build failure?

[mtyaka]

@antoviaque Yes, but the integration tests are quite flaky. Travis runs two builds per PR - a "merge" build and a "pr branch" build. In this case one of the builds was successful, while the other was not - even though they were both running the same code (the PR branch was rebased against master). The tests also pass locally for me. We might want to investigate why the tests are so flaky, I got random travis build failures about half of the time when trying to get to green (without changing any code).

[antoviaque]

@mtyaka In this cases there are different things that can be done:

• Fix the flaky test - here since the test I saw was failing was one I think was introduced by the current PR, it would have made sense imho (test_scroll_into_view).
• Get the sprint fireman and/or the person who wrote the test to have a look at it ( @itsjeyd would you have time to help here? )
• Disable the flaky test & add a bug about it the backlog - not ideal, but if all else fails, this is still better than a broken build

Happy to discuss what the approach/steps should be in these cases, but it would be good if we had a solution that didn't involve merging broken builds. Otherwise, it will progressively undermine our confidence in the build, and it will start to rot -- it won't be long before people start merging builds with genuine failures ("it always fails anyway...")

[mtyaka]

@antoviaque You're right, the last failure was in the new test. I didn't notice that. It's not the only flaky test, though. I've had random failures in multiple tests. One that I've (randomly) seen multiple times was MultipleObjectsReturned: get() returned more than one XBlockState - no idea what that's about. There was also one about some buttons not showing, completely unrelated to changes in this PR.

I think it would be nice if we add a task in the backlog to make tests more robust. It would also be great if we could figure out what all of the random 'broken pipe' errors mean and get rid of them.

[antoviaque]

@mtyaka Yup, if you add a ticket in the backlog, I'll try to put it toward the top. But it will be hard to fix it once for all imho, so I'd also like to make sure, at the same time, that we know how to handle these cases in the future. Maybe something to bring up during the meeting today?

[itsjeyd]

@antoviaque @mtyaka I think we already have a ticket for this: OC-807. For the "Broken pipe" thing, this might help.

[mtyaka]

Thanks for finding that ticket @itsjeyd

@antoviaque Yeah, we can discuss it on the call. I agree we need to take this more seriously. I think all three suggestions make sense - depending on how much time the author and fireman have. Most often it isn't obvious what causes flakiness, so it's often hard to estimate time fixing it will take, especially considering the fact that integration tests take ~30 minutes to run. If at all possible, we should try to reduce that. I don't really have any ideas other than try to replace the majority of the integration tests with JS tests.

[antoviaque]

@itsjeyd Thanks! My bad actually, I hadn't realized we had this ticket, forgotten in the backlog; pulling it back up.

@mtyaka And yup, sounds good. And I agree that some of the integration tests could be JS tests - even without converting them, that could be something to consider in future tasks, to prefer implementing them as JS tests, and keep integration tests for when they are really useful.