Page MenuHomePhabricator

Fix failed UploadWizard browsertests Jenkins job
Closed, DeclinedPublic

Event Timeline

Restricted Application added a project: Multimedia. · View Herald TranscriptMar 27 2015, 12:45 PM
Gilles added a subscriber: Gilles.Mar 27 2015, 3:04 PM

Same remarks as the ones I left on T94157 apply here. It's not realistic to hold us up to that standard, given that this is only an area we could focus on every few weeks.

Ok, looks like somebody cares about browser tests, so we will not delete any jobs. :) Feel fee to reach out to Release-Engineering-Team team if you need help. Also, I have proposed Workshop: Fix broken browsertests/Selenium Jenkins jobs (T94299) for this year's hackathons.

zeljkofilipin renamed this task from Delete or fix failed UploadWizard browsertests Jenkins job to Fix failed UploadWizard browsertests Jenkins job .Mar 30 2015, 2:01 PM
zeljkofilipin set Security to None.

I don't have plans yet for the Lyon hackathon, I'll be very happy to participate in that and recruit victims/volunteers interested in learning about writing browser tests.

Jdforrester-WMF moved this task from Untriaged to Backlog on the Multimedia board.Sep 4 2015, 6:25 PM
Restricted Application added subscribers: Steinsplitter, Matanya. · View Herald TranscriptSep 4 2015, 6:25 PM
matmarex closed this task as Declined.Feb 12 2016, 5:26 PM
matmarex claimed this task.
matmarex added a subscriber: matmarex.

Mark says that nobody cares and that we're going to use something new for browser tests soon. Pretty sure the old ones are completely hopeless after all the UI changes. Please delete them.

Change 270720 had a related patch set uploaded (by Zfilipin):
Delete jobs for UploadWizard Selenium tests

https://gerrit.wikimedia.org/r/270720

Change 270720 merged by jenkins-bot:
Delete jobs for UploadWizard Selenium tests

https://gerrit.wikimedia.org/r/270720

I guess we should delete the test/browser/ directory in rEUWI now?

Change 270758 had a related patch set uploaded (by Bartosz Dziewoński):
Remove browser tests

https://gerrit.wikimedia.org/r/270758

Change 270764 had a related patch set uploaded (by Zfilipin):
Do not run rake-jessie for UploadWizard

https://gerrit.wikimedia.org/r/270764

Change 270764 merged by jenkins-bot:
Do not run rake-jessie for UploadWizard

https://gerrit.wikimedia.org/r/270764

Change 270758 merged by jenkins-bot:
Remove browser tests

https://gerrit.wikimedia.org/r/270758

Mark says that nobody cares and that we're going to use something new for browser tests soon. Pretty sure the old ones are completely hopeless after all the UI changes. Please delete them.

Ah, found the context that matters. For the record, I care about months of work and by extension tens of thousands of dollars of donor money going to waste because you guys couldn't be bothered doing minor maintenance like changing CSS selectors. I don't see anything so drastic in the new UI that would make the "old" tests "hopeless".

But if "something new for browser tests" is there with equal coverage, it's a lesser evil I guess. Unless of course it's going to take just as long to write new browser tests from scratch. In which case it's donor money wasted for the sake of rewriting something that does the same thing as what there before on a new tool.

Speaking of "soon", that was 8 months ago. Do the new browser tests exist yet?

No, no new browser tests exist.

To my knowledge, the old browser tests saved us from zero bugs since I joined Multimedia. They were anything but complete. They definitely did not prevent the numerous interface-breaking bugs in the code I inherited (like the "Next" button disappearing after specific sequences of user actions, or JS exceptions being thrown all over the place), and which I since fixed.

Personally I think Cucumber-style tests are a pain to write and a pain to read. And it looks like you weren't a big fan of maintaining them either, based on your earlier comments on this task.

If you really spent "tens of thousands of dollars" writing them then that's a shame, but I value work based on the results and not based on the effort. We really should be bolder about deleting code as an organization.

Gilles added a comment.Nov 3 2016, 9:59 AM

It wasn't a matter of "being a fan" of maintaining them. It wasn't my responsibility to maintain them, it was yours. A responsibility you've failed by obliterating cross-browser integration code coverage, bringing it to zero.

I only had to look back a week's worth of bugs to find one that these tests would have caught.

Arguing for deleting of tests as if it's something courageous is a level of incompetence that defies belief.

Gilles added a comment.EditedNov 3 2016, 10:07 AM

I wouldn't be so hard on this matter had the tests been replaced by something better, but they've been replaced by nothing. Judging by the results, as you say, a net negative has been produced. At the very least bug fixes like T148782 should come with a test case of some kind (unit test, integration test, whatever you like written in any tool of your liking) to avoid the exact same issue reappearing. But it doesn't, which is the symptom of a cultural failing where tests are not only not written, they are deleted because the simple task of maintaining them is seen as pointless.

You are creating a buggy experience for users with this work practice, whether that's pleasant to hear or not. Why isn't your response that you have a better solution in mind to catch regression bugs? What I would like to hear is that eventually a net positive will be reached, which piece of code survives or not is less relevant.

What is unfair in this matter, is not what I'm stating now, but just like earlier in this task, the burden to fix, prove the worth of, maintain and all around give a damn about integration tests is entirely on me. I feel like at this point the only way to convince you would be to spend a week resurrecting those tests, fixing them. Basically doing what you should have done since inheriting UploadWizard. Then going and finding how many past live bugs they would have caught. And then maybe your opinion might change to the point of reconsidering your current practice.

Just to compensate for what should have happened, i.e. taking a real interest in the work that had been done in that area. Possibly converting it to something else than cucumber if you found that too hard to work with.

All I can provide today is a rant with the hope of causing some self-reflection, because I have a ton of other responsibilities now and UW hasn't been part of mine for a long time. It's just not possible to stand, watch and say nothing.

(I know it's bad form to respond to individual sentences, but you're making many points and I hope I'm not losing any context by this.)

Arguing for deleting of tests as if it's something courageous is a level of incompetence that defies belief.

That is not what I am arguing.

Even if I was, I don't think this would even make it to the top ten of most belief-defyingly incompetent things we've done as an organization. :)

I only had to look back a week's worth of bugs to find one that these tests would have caught.

For reference, this is about T148782. One example proves nothing. If you looked back a few weeks more, I doubt you'd find any more.

Also, perhaps I am in fact being incompetent here, but how would those tests have caught this? Nothing in them seems to verify the presence or absence of that "Or" element. Is there something magical happening that I'm overlooking?

It wasn't a matter of "being a fan" of maintaining them. It wasn't my responsibility to maintain them, it was yours. A responsibility you've failed by obliterating cross-browser integration code coverage, bringing it to zero.

I think one of the responsibilities of maintainers is to "sunset" products when they are no longer worth maintaining. As far as I could tell, the browser tests were not helping us.

Also, the tests are not "obliterated". You can bring them back with one Git command. (But it's true that they probably bitrotted more than they would otherwise while being deleted.)

There is great value in having tests, but good development practices can compensate for a lack of automated testing. (And automated testing can compensate for a lack of good development practices.) They're not the end-all, be-all of software development. I don't subscribe to the tenets of TDD. :)

I think the problems that plague UploadWizard are mostly caused by poor architecture, not lack of unit/integration testing. There have been many issues where the interface state goes out of sync with internal data, or where the interface state becomes broken so that the user can't proceed. I believe this is mostly because the code tries to update both based on user input whenever something happens, rather than update internal data based on user input and interface based on the data. (Incidentally, this architecture also makes unit testing more difficult, since it forces you to simulate user input rather than changes to the internal data.) We've done some work towards this, but it's hard to retrofit without rewriting the whole system. (If you like design patterns, this is like model-view-controller, but view and controller are mostly merged. VisualEditor is an good example of this.)

In spite of your assertions about buggy experiences, UploadWizard is currently in the best condition is has ever been. We've gotten the number of exceptions being thrown from exorbitant to near-zero (T136230: Get UploadWizard's uncaught exceptions on Commons to fewer than 50/day). We've improved user success rate in uploading files (T150010: Review UploadWizard funnel (click tracking) data again – thanks for motivating me to do this analysis). Anecdotally, it feels like the number of issues being reported at Commons: Upload Wizard feedback and Commons: Help desk is also far lower, and when they appear they're often caused by API bugs and server issues.

I am glad you care, but I really don't think the picture is as grim as you paint it.

hashar added a comment.Nov 4 2016, 8:34 PM

... I think Cucumber-style tests are a pain to write and a pain to read. ...

I agree Cucumber is confusing but it is not required, one can write the tests directly using the ruby selenium-webdriver.

There are a few comprehensive examples written by @zeljkofilipin at https://www.mediawiki.org/wiki/Selenium/Ruby each example introducing a new piece of the stack (selenium, watir, page-object, watir).

There is some experiment going on to use JavaScript bindings instead of ruby. You can read about it at https://www.mediawiki.org/wiki/Selenium/JavaScript

Browser tests have their value, but not if they are not maintainable. Maybe it would be worth porting the existing one to JavaScript if that is easier to handle?

I am really glad that we are discussing test automation, but I am not glad with the tone of the conversation.

I do agree that Ruby stack was too complicated, and I am probably the most responsible for that. At the time, I have thought it was the best way to implement Selenium tests.

@matmarex, if you would like to implement a simpler Selenium tests in Node.js (using only Selenium and Mocha), please take a look at Selenium/JavaScript page. I would be glad to pair on it for a few hours a week.

For general advice on how to get help with Selenium, please see Get help section of Selenium page. (I am zeljkof in #wikimedia-releng.)