Refactor Aggregation Tests

[kidrahahjo]

Description

Refactor tests written for aggregates.py

Motivation and Context

Refer to #611

Checklist:

• I am familiar with the Contributing Guidelines.
• I agree with the terms of the Contributor Agreement.
• My name is on the list of contributors.
• The changes introduced by this pull request are covered by existing or newly introduced tests.
• The package documentation and framework documentation in signac-docs are up to date with these changes.
• I have updated the changelog and added any related issue and pull request numbers for future reference.

[codecov]

Codecov Report

Merging #645 (9fd79af) into master (b550ffa) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #645   +/-   ##
=======================================
  Coverage   79.84%   79.84%           
=======================================
  Files          33       33           
  Lines        3195     3195           
  Branches      682      682           
=======================================
  Hits         2551     2551           
  Misses        507      507           
  Partials      137      137           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[b-butler]

@kidrahahjo I like the idea of making our test files more modular, but I would caution against using a directory named common, misc, etc. as these names are fairly ambiguous. I think that matching the module structure generally works best. Here where we have one monster file, I think splitting these tests into multiple files is fine.

[kidrahahjo]

@kidrahahjo I like the idea of making our test files more modular, but I would caution against using a directory named common, misc, etc. as these names are fairly ambiguous. I think that matching the module structure generally works best. Here where we have one monster file, I think splitting these tests into multiple files is fine.

Okay! I will make it similar to that of the module.

[kidrahahjo]

This PR should then do some additional work as well. I think refactoring aggregation tests would be great for the scope of this PR.

[kidrahahjo]

Revert unrelated change

[kidrahahjo]

Requesting @b-butler to review this, since I've written most of the aggregation tests with him.

[b-butler]

I left some comments and suggestions to address.

[b-butler]

Just call aggregator_instance._create_AggregatorStore(project) directly.

[kidrahahjo]

I could call it directly, but it makes the code look more clean IMO. Happy to change this if you want

[kidrahahjo]

The reason I'm stating this though is that if I remove this then I'll have to add multiple lines of code for initializing the aggregator_instance first. It might be painful to do that but if the current one is against best practices then I'm happy to change this.
@b-butler what do you suggest?

[b-butler]

Suggested change

	assert list(aggregate_instance._aggregator_function(agg_value)) == [
	tuple(agg_value)
	]
	assert next(aggregate_instance._aggregator_function(agg_value)) == tuple(agg_value)

Also, this check seems a bit off to me. I get that we just wrap anything passed in a tuple by default, but this isn't really what the default aggregator is for. At the very least we should comment why this check exists.

[kidrahahjo]

I don't think we should be choosing the first element here using next because we need to make sure that the output of _aggregator_function is everything grouped into 1.

[b-butler]

I get your thoughts, just testing the aggregator on random input feels off. We also don't need to parameterize this test.

[kidrahahjo]

Hmm, let's remove the parameters but are you suggesting to remove this test instead?

[kidrahahjo]

I removed the parameters, let me know how it looks

[tcmoore3]

Looks good. Approving since addressing Brandon's comments should be sufficient.

[vyasr]

@b-butler how does everything here look to you now? It looks like you're pretty deep into the review already and it's probably easier for you to finish than to bring in anyone else, but feel free to ping if it needs another eye.

[b-butler]

👍

[kidrahahjo]

Merging this as it has two approvals.