Create tests for each event constructor #186
Conversation
| import 'dart:mirrors'; | ||
|
|
There was a problem hiding this comment.
@bkonyi i know using this package is a little "iffy", but in the context of this test file, i needed a way to ensure that each constructor was being tested from the Event class.
Thoughts?
There was a problem hiding this comment.
Ehhhhhh, I wouldn't encourage use of dart:mirrors but I don't have an alternative for you. If this functionality works, you're probably fine to use it, but lots of dart:mirrors has been left to bit-rot and won't work with newer language features (anything >= Dart 2.0 could be partly broken or might just not work).
There might be a way to do this with package that utilizes code gen, but I'm not sure.
| var constructorCount = 0; | ||
| for (var declaration in reflectClass(Event).declarations.keys) { | ||
| if (declaration.toString().contains('Event.')) constructorCount++; | ||
| } | ||
|
|
||
| // Change this integer below if your PR either adds or removes | ||
| // an Event constructor | ||
| final eventsAccountedForInTests = 17; | ||
| expect(eventsAccountedForInTests, constructorCount, | ||
| reason: 'If you added or removed an event constructor, ' | ||
| 'ensure you have updated ' | ||
| '`pkgs/unified_analytics/test/event_test.dart` ' | ||
| 'to reflect the changes made'); |
There was a problem hiding this comment.
This is the only test that uses the dart:mirrors by using reflectClass(...)
| // Change this integer below if your PR either adds or removes | ||
| // an Event constructor | ||
| final eventsAccountedForInTests = 17; |
There was a problem hiding this comment.
This test in particular strikes me as purely a change detection test. Should we instead trust future contributors and code reviewers to make sure the constructors are under test as they add/delete code?
There was a problem hiding this comment.
I think in this case it would make sense though because a contributor could forget to add a test for a newly added Event constructor, in which case this test would fail in presubmit checks so it can save time for a reviewer to flag.
Should this be "closes #182"? |
Yep, I just have a bad habit of always writing related |
Closes issue:
Contribution guidelines:
dart format.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.