Page MenuHomePhabricator

Delete tests disabled for more than 1 year
Open, Needs TriagePublic

Description

Problem

Every time we do a major framework improvement, it breaks existing code. (For a list of major framework improvements, see J317.) We usually update all broken code. If a test is disabled, it doesn't run, it doesn't break when the upgrade happens, so we don't fix it. After a year or two, there will be one or two major framework updates. If the code is enabled after being disabled for a year or two, there's a high chance it will not work.

The alternative to not updating disabled tests is enabling them during the migration. We make sure they work with the current framework, then we disable them again. That makes sure the test can be enabled, but it also wastes our time on code that nobody might care about any more.

Solution

  • Commented out code or skipped tests should not be in our codebase.
  • If a reminder for a team is needed, a task should be created. If the test is deleted, the code is not deleted from this universe. It's still in version control. The task can point to the commit that deletes the code. It's easy to get the code back.
  • 1 year should be enough time to fix a test.
  • If a test is not useful enough to be disabled for a year, it should be deleted.
  • I'm open to making the deadline shorter than 1 year.
  • when deleting the test, add to reviewers
    • the person that wrote it
    • the person that disabled it
    • people that made changes to it

Details

SubjectRepoBranchLines +/-
mediawiki/coremaster+0 -50
mediawiki/extensions/VisualEditormaster+0 -8
mediawiki/extensions/GrowthExperimentsmaster+0 -115
mediawiki/extensions/Echomaster+1 -28
mediawiki/extensions/CampaignEventsmaster+0 -40
mediawiki/extensions/AbuseFiltermaster+0 -6
mediawiki/extensions/Wikibasemaster+0 -66
mediawiki/extensions/GrowthExperimentsmaster+1 -118
mediawiki/extensions/CodeMirrormaster+0 -38
mediawiki/skins/MinervaNeuemaster+0 -97
mediawiki/skins/MinervaNeuemaster+1 -0
mediawiki/extensions/Wikibasemaster+2 -0
mediawiki/coremaster+1 -1
mediawiki/extensions/GrowthExperimentsmaster+3 -0
mediawiki/skins/MinervaNeuemaster+3 -2
mediawiki/extensions/VisualEditormaster+1 -0
integration/configmaster+1 -0
mediawiki/extensions/CodeMirrormaster+1 -0
mediawiki/skins/MinervaNeuemaster+1 -0
mediawiki/extensions/CampaignEventsmaster+1 -0
mediawiki/extensions/AbuseFiltermaster+1 -0
mediawiki/extensions/Echomaster+1 -0
mediawiki/extensions/MobileFrontendREL1_40+3 -0
mediawiki/extensions/MobileFrontendREL1_39+2 -3 K
mediawiki/extensions/WikibaseLexememaster+0 -38
mediawiki/skins/MinervaNeuemaster+0 -34
mediawiki/extensions/MobileFrontendmaster+2 -3 K
mediawiki/skins/MinervaNeuemaster+1 -31
mediawiki/extensions/RelatedArticlesmaster+1 -6
mediawiki/extensions/EntitySchemamaster+0 -34
mediawiki/extensions/WikibaseLexememaster+0 -17
integration/configmaster+0 -3
mediawiki/extensions/MobileFrontendmaster+1 -1
mediawiki/extensions/Wikibasemaster+1 -1
mediawiki/extensions/Echomaster+1 -1
mediawiki/coremaster+1 -1
mediawiki/extensions/EntitySchemamaster+0 -18
mediawiki/extensions/EntitySchemamaster+4 -4
mediawiki/skins/MinervaNeuemaster+4 -34
mediawiki/coremaster+0 -18
mediawiki/extensions/Wikibasemaster+0 -55
mediawiki/extensions/GrowthExperimentsmaster+0 -16
mediawiki/coremaster+0 -161
mediawiki/extensions/FileImportermaster+1 -12
mediawiki/extensions/WikibaseLexememaster+0 -39
mediawiki/extensions/Echomaster+0 -32
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 862310 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/skins/MinervaNeue@master] selenium: Delete failing test

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

Change 862310 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] selenium: Delete failing test

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

Change 859505 abandoned by Zfilipin:

[mediawiki/extensions/WikibaseLexeme@master] selenium: Delete tests disabled for more than 1 year

Reason:

The tests seem to be fixed with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseLexeme/+/861355

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

zeljkofilipin updated the task description. (Show Details)
zeljkofilipin updated the task description. (Show Details)
zeljkofilipin moved this task from Deep work 🌊 to Waiting 🏹 on the User-zeljkofilipin board.
zeljkofilipin changed the task status from In Progress to Open.Dec 9 2022, 12:47 PM

Change 952002 had a related patch set uploaded (by Jforrester; author: Zfilipin):

[mediawiki/extensions/MobileFrontend@REL1_39] selenium: Delete all tests

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

Change 952002 merged by Jforrester:

[mediawiki/extensions/MobileFrontend@REL1_39] selenium: Delete all tests

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

Change 955803 had a related patch set uploaded (by Jforrester; author: Zfilipin):

[mediawiki/extensions/MobileFrontend@REL1_40] selenium: Delete all tests

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

Change 955803 abandoned by Jforrester:

[mediawiki/extensions/MobileFrontend@REL1_40] selenium: Delete all tests

Reason:

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

zeljkofilipin raised the priority of this task from Medium to Needs Triage.Oct 31 2023, 4:21 PM
Jbol updated the task description. (Show Details)

Change #1052733 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/AbuseFilter@master] selenium: Document when, how and why a test is skipped

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

Change #1052738 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/Echo@master] selenium: Document when, how and why a test is skipped

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

zeljkofilipin changed the task status from Open to In Progress.Jul 10 2024, 3:10 PM
zeljkofilipin triaged this task as Low priority.

Change #1053705 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/CampaignEvents@master] selenium: Document when, how and why a test is skipped

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

Change #1052738 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] selenium: Document when, how and why a test is skipped

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

Change #1052733 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] selenium: Document when, how and why a test is skipped

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

Change #1053705 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] selenium: Document when, how and why a test is skipped

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

Change #1054075 had a related patch set uploaded (by Shyblumer; author: Shyblumer):

[mediawiki/skins/MinervaNeue@master] selenium: Add date and bug ID to skipped tests

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

Change #1054343 had a related patch set uploaded (by Jbol; author: Jbol):

[mediawiki/extensions/CodeMirror@master] selenium: Document when, how and why a test is skipped

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

Change #1054075 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] selenium: Add date and bug ID to skipped tests

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

Change #1054343 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@master] selenium: Document when, how and why a test is skipped

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

Change #1064793 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[integration/config@master] zuul: Add John Bolorinos to trusted users

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

Change #1064793 merged by jenkins-bot:

[integration/config@master] Zuul: Add John Bolorinos to trusted users

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

Change #1073210 had a related patch set uploaded (by Jbol; author: Jbol):

[mediawiki/extensions/VisualEditor@master] selenium: Document when, how and why a test is skipped

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

Change #1073210 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] selenium: Document when, how and why a test is skipped

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

Change #1077327 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/skins/MinervaNeue@master] selenium: Document when, how and why a test is skipped

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

Change #1077328 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/GrowthExperiments@master] selenium: Document when, how and why a test is skipped

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

Change #1077331 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/Wikibase@master] selenium: Document when, how and why a test is skipped

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

Change #1077334 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/core@master] selenium: Document when, how and why a test is skipped

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

Change #1077327 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] selenium: Document when, how and why a test is skipped

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

Change #1077328 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] selenium: Document when, how and why a test is skipped

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

Change #1077334 merged by jenkins-bot:

[mediawiki/core@master] selenium: Document when, how and why a test is skipped

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

Change #1077331 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] selenium: Document when, how and why a test is skipped

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

Change #1078414 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/skins/MinervaNeue@master] selenium: Document when, how and why a test is skipped

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

Change #1078414 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] selenium: Document when, how and why a test is skipped

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

Change #1080246 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/CodeMirror@master] selenium: Delete 'CodeMirror bracket match highlighting for the wikitext 2017 editor'

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

Change #1080597 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/skins/MinervaNeue@master] selenium: Delete tests disabled more than a year ago

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

Change #1080597 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] selenium: Delete tests disabled more than a year ago

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

Change #1081080 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/GrowthExperiments@master] selenium: Delete tests disabled more than a year ago

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

Change #1083857 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/AbuseFilter@master] selenium: Delete tests disabled six months ago

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

Change #1083859 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/CampaignEvents@master] selenium: Delete tests disabled six months ago

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

Change #1083861 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/Echo@master] selenium: Delete tests disabled more than two years ago

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

if a test is not useful enough to be disabled for a year, it should be deleted

I don't agree with this. Teams are busy. If you delete the test, it means there's almost zero chance of someone being able to go back and fix a flaky test that otherwise provided useful value in preventing regressions. I would leave them in place as documentation. I don't think it harms anything to leave them around.

if a test is not useful enough to be disabled for a year, it should be deleted

I don't agree with this. Teams are busy. If you delete the test, it means there's almost zero chance of someone being able to go back and fix a flaky test that otherwise provided useful value in preventing regressions. I would leave them in place as documentation. I don't think it harms anything to leave them around.

I just picked 1 year as a reasonable time without much thought. If you think longer time is more reasonable, I'm open to discussion. (I think that time should be much shorter. 3 or 6 months.) After how long would it be reasonable to conclude nobody cares about a test? 2 years? 3 years?

I don't agree that commented out code or skipped tests should be in our codebase. If a reminder for a team is needed, a task should be created. If the test is deleted, the code is not deleted from this universe. It's still in version control. The task can point to the commit that deletes the code. It's easy to get the code back.

I think I never explicitly stated the problem this task is trying to solve. Every time we do a major framework improvement, it breaks existing code. (For a list of major framework improvements, see Blog Post: Refactoring WebdriverIO Tests From Sync to Async Mode.) We usually update all broken code. If a test is disabled, it doesn't run, it doesn't break when the upgrade happens, so we don't fix it. After a year or two, there will be one or two major framework updates. If the code is enabled after being disabled for a year or two, there's a high chance it will not work.

The alternative to not updating disabled tests is enabling them during the migration. We make sure they work with the current framework, then we disable them again. That makes sure the test can be enabled, but it also wastes our time on code that nobody might care about any more.

Change #1084065 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/VisualEditor@master] selenium: Delete a test disabled more than a year ago

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

Change #1084066 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/extensions/Wikibase@master] selenium: Delete a test disabled almost a year ago

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

Change #1084068 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/core@master] selenium: Delete a test disabled almost two years ago

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

zeljkofilipin changed the task status from In Progress to Open.Oct 30 2024, 10:21 AM
zeljkofilipin removed zeljkofilipin as the assignee of this task.
zeljkofilipin raised the priority of this task from Low to Needs Triage.
zeljkofilipin moved this task from Deep work 🌊 to Q2 👔 on the User-zeljkofilipin board.

Change #1080246 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@master] selenium: Delete 'CodeMirror bracket match highlighting for the wikitext 2017 editor'

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

if a test is not useful enough to be disabled for a year, it should be deleted

I don't agree with this. Teams are busy. If you delete the test, it means there's almost zero chance of someone being able to go back and fix a flaky test that otherwise provided useful value in preventing regressions. I would leave them in place as documentation. I don't think it harms anything to leave them around.

I just picked 1 year as a reasonable time without much thought. If you think longer time is more reasonable, I'm open to discussion. (I think that time should be much shorter. 3 or 6 months.) After how long would it be reasonable to conclude nobody cares about a test? 2 years? 3 years?

I'd leave it indefinitely.

I don't agree that commented out code or skipped tests should be in our codebase. If a reminder for a team is needed, a task should be created. If the test is deleted, the code is not deleted from this universe. It's still in version control. The task can point to the commit that deletes the code. It's easy to get the code back.

It's documentation of something that, at some point, an individual or team thought was important. If it's disabled, it's due to a failure of implementation, but that doesn't take away from the importance of the test. For Selenium specifically, it means at a certain point the effort of trying to debug our WDIO setup is not worth the value we'd get out of the test.

I think I never explicitly stated the problem this task is trying to solve. Every time we do a major framework improvement, it breaks existing code. (For a list of major framework improvements, see Blog Post: Refactoring WebdriverIO Tests From Sync to Async Mode.) We usually update all broken code. If a test is disabled, it doesn't run, it doesn't break when the upgrade happens, so we don't fix it. After a year or two, there will be one or two major framework updates. If the code is enabled after being disabled for a year or two, there's a high chance it will not work.

The alternative to not updating disabled tests is enabling them during the migration. We make sure they work with the current framework, then we disable them again. That makes sure the test can be enabled, but it also wastes our time on code that nobody might care about any more.

I see. Then deleting makes sense. But alternatively, perhaps we could invest the effort in making the disabled tests more resilient and less flaky?

Change #1089338 had a related patch set uploaded (by Jared Blumer; author: Jared Blumer):

[mediawiki/extensions/GrowthExperiments@master] Remove skipped tests in addImage.js and delete homepage.js

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

Change #1081080 abandoned by Zfilipin:

[mediawiki/extensions/GrowthExperiments@master] selenium: Delete tests disabled more than a year ago

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

Change #1084066 abandoned by Zfilipin:

[mediawiki/extensions/Wikibase@master] selenium: Delete a test disabled almost a year ago

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

Change #1083857 abandoned by Zfilipin:

[mediawiki/extensions/AbuseFilter@master] selenium: Delete tests disabled six months ago

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

Change #1083859 abandoned by Zfilipin:

[mediawiki/extensions/CampaignEvents@master] selenium: Delete tests disabled six months ago

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

Change #1089338 abandoned by Zfilipin:

[mediawiki/extensions/GrowthExperiments@master] Remove skipped tests in addImage.js and delete homepage.js

Reason:

Abandoning since the test will be moved to cypress.

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

Change #1083861 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] selenium: Delete tests disabled more than two years ago

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