Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

gps tests: dry and cleanup; remove unused code#620

Merged
sdboyer merged 2 commits intogolang:masterfrom
jmank88:test_cleanup
May 23, 2017
Merged

gps tests: dry and cleanup; remove unused code#620
sdboyer merged 2 commits intogolang:masterfrom
jmank88:test_cleanup

Conversation

@jmank88
Copy link
Copy Markdown
Collaborator

@jmank88 jmank88 commented May 22, 2017

This is a small change to gps tests which removes some unused code, and cleans-up some repeated sub-test code which was unintentionally serial.

do(typ, fixtures, t)
})
}
t.Run("second", runSet)
Copy link
Copy Markdown
Collaborator Author

@jmank88 jmank88 May 22, 2017

Choose a reason for hiding this comment

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

do calls t.Run with t.Parallel, but since each do was called from a separate t.Run, those calls would block until the single test completed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh derp yes, thank you 👍

// just in case something needs punishing, kubernetes is happy to oblige
kub = atom{
id: pi("github.com/kubernetes/kubernetes"),
v: NewVersion("1.0.0").Is(Revision("528f879e7d3790ea4287687ef0ab3f2a01cc2718")),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's leave this in; as the comment indicates, it's there mostly to do the work of picking out a known kubernetes rev to work from. Even though it's not currently used anywhere, simply making that choice has some potential value down the road.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Is dead code the best way to document that choice? What about commenting out the block?

Is there any other rationale for this particular revision, other than that its 'known to work'?

// Just in case something needs punishing, kubernetes offers a complex,
// real-world set of dependencies, and this revision is known to work.
/*
	_ = atom{
		id: pi("github.com/kubernetes/kubernetes"),
		v:  NewVersion("1.0.0").Is(Revision("528f879e7d3790ea4287687ef0ab3f2a01cc2718")),
	}
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Commenting it out is also OK, I suppose.

The biggest reason to pick a revision is just the chaos that results from suddenly this, and having people pick their own random revisions. Or, worse, a branch tip.

Basically, this is me just trying to proactively head off a coordination problem, should it arise.

do(typ, fixtures, t)
})
}
t.Run("second", runSet)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh derp yes, thank you 👍

@sdboyer sdboyer merged commit f850240 into golang:master May 23, 2017
@sdboyer
Copy link
Copy Markdown
Member

sdboyer commented May 23, 2017

thanks! 🎉

@jmank88 jmank88 deleted the test_cleanup branch June 7, 2017 23:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants