Partition benchmark run info based on added nuget packages

[blairconrad]

Fixes #931. Related to #922.

I tried to follow the local coding style, but my have failed. In particular, I saw no consistent method for calculating hashes. I'm happy to make any changes to conform.

The partitioner needs an implementation where package references are considered different if either name or version differ, so I amended Equals and GetHashCode. then HashSet wouldn't recognize duplicate packages, so I replaced it with a custom collection. I had it throw when the nuget reference's package name is a duplicate, since configuring different package versions of the same package for one job is an error, and it seems better to fail fast rather than run with perhaps an unintended version.

Thanks for considering this. Again, I'm happy to make any changes at all.

[AndreyAkinshin]

@Shazwazza, @adamsitnik could you please review this PR?

[adamsitnik]

Thanks for spending your time and fixing this bug!

The only thing I would like you to change before I merge this fix is:

- hashCode ^= job.Infrastructure.GetHashCode();
+ hashCode ^= job.Infrastructure.NugetReferences.GetHashCode();

I am also not sure if we should have a custom collection of NugetReference type or just give up on the idea of NugetReference and use Dictionary<string, string> which would solve all of our problems (except of the primitive obsession). But we can leave that for later.

[adamsitnik]

Suggested change

	hashCode ^= job.Infrastructure.GetHashCode();
	hashCode ^= job.Infrastructure.NugetReferences.GetHashCode();

[adamsitnik]

hmm, maybe we should give up on the idea of NugetReference and use Dictionary<string, string> instead?

[blairconrad]

The only thing I would like you to change before I merge this fix is:

- hashCode ^= job.Infrastructure.GetHashCode();
+ hashCode ^= job.Infrastructure.NugetReferences.GetHashCode();

Of course. So glad you caught this. Copy/paste error. I will definitely fix. Do you prefer an amended commit or a fixup! to be later squashed?

I would also be happy to explore the use of a Dictionary (I'm assuming package name is the key and the version the value) for the nuget packages, if that's your preference. Although I'm not sure it would solve all our problems, at least without also adding

1. an up-front check to ensure we don't replace an existing entry with a new one that has a different version, which is currently encapsulated in NugetReferenceList, and
2. custom equality-checking and hashcode-generation, which is currently encapsulated in NugetReferenceList.

but if you prefer that code to be separate from the container, I can make the change.

[blairconrad]

Whoops. Builds failed because I'd left NugetReferenceList as internal. I had an unpushed change in my local repo that resolves this, but will wait on it before hearing your feelings on amendments vs. fixup commits (and also wondering if my Dictionary comments will turn you back onto the NugetRefernceList: if not, the problem goes away anyhow).

[adamsitnik]

You don’t need to squash, I can do it from github UI level when merging the PR. When it comes to using the dictionary we can leave it for later. I will do some PoC myself to validate this idea.

[Shazwazza]

I didn't realize changes were needed for the BenchmarkPartitioner, wasn't code that i had actually looked at, clearly should have tested further ;) And looks like a problem snuck in there with my later changes to the HashSet of NugetReference. Good to see there wasn't a ton of code that needed changing.

Only nitpick might be that if we keep the NugetReferenceList maybe we should make it implement IReadOnlyList instead of IReadOnlyCollection since it seems that most other collections are exposed as this and means in the test https://github.com/dotnet/BenchmarkDotNet/pull/922/files#diff-048cab01ff351e9c6602501a273532d5R480 that ElementAt doesn't need to be used an instead an indexer can be used. I ended up using IReadOnlyCollection only due to the HashSet usage since it doesn't implement IReadOnlyList

[blairconrad]

I see that AppVeyor failed, but do not see an actual failure. Does the build time out after an hour? (Yup. Found the message.) I can't imagine what in my change would blow the time up, especially considering that the Travis build was as fast as usual.

[adamsitnik]

@blairconrad thank you! the CI failure seems to be unrelated, don't worry

[blairconrad]

Yay! Thanks for the reviews and merge, folks.