Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lag : Add test #667

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Lag : Add test #667

wants to merge 1 commit into from

Conversation

Orace
Copy link
Contributor

@Orace Orace commented Nov 8, 2019

Fix a part of #649
Fork of #660

Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

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

I am struggling to understand the value proposition of this test. If it's to make sure that TestingSequence is used then why not add its use to the existing tests instead of isolating it to one test?

@Orace
Copy link
Contributor Author

Orace commented Nov 9, 2019

In unit testing pattern we should not even use TestingSequence since it tests multiple things. (Enumerator disposal, multiple enumeration, multiple MoveNext at the end of the source).

I'm agree that TestLagPassesCorrectLagValuesOffsetBy1 and TestLagPassesCorrectLagValuesOffsetBy2 can be replaced by new cases in TestLagOnKnownInput

The proper (in terms of unit testing) way to work is to split TestingSequence in 3 new mock (TestingSequenceForDisposed, etc..), add one test for each of it, add relevant cases for each of it.

I don't really want to push that since it's a lot of rework..

@Orace Orace mentioned this pull request Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants