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

Fix for test-runner is used before being assigned #396

Conversation

winksaville
Copy link
Contributor

Without this change TSC is correctly giving warnings that
currentTestFixtureResults and currentTestResults are
used before being assigned.

Also, with this change but not using stricter compiler option test coverage
is missing for line 89, which means currentTestResults is always valid.
But if I put that then portion of if (currentTestResults) { into
if (currentTestFixtureResults && (!previousTestItem || previousTestItem.test !== testItem.test)) {
_runTests will fail.

It seems this loop is a little odd and maybe needs some refractoring, but
I'm not sure exactly how it works so I need some guidance.

Description

Checklist

  • I am an awesome developer and proud of my code
  • I added / updated / removed relevant unit or integration tests to prove my change works
  • I ran all tests using npm test to make sure everything else still works
  • I ran npm run review to ensure the code adheres to the repository standards

Additional Information

Without this change TSC is correctly giving warnings that
`currentTestFixtureResults` and `currentTestResults` are
`used before being assigned`.

Also, with this change but not using stricter compiler option test coverage
is missing for line 89, which means `currentTestResults` is always valid.
But if I put that `then` portion of `if (currentTestResults) {` into
`if (currentTestFixtureResults && (!previousTestItem || previousTestItem.test !== testItem.test)) {`
`_runTests` will fail.

It seems this loop is a little odd and maybe needs some refractoring, but
I'm not sure exactly how it works so I need some guidance.
@winksaville
Copy link
Contributor Author

@jamesrichford or @Jameskmonger, as I said in the commit message, the loop is a little weird so maybe there is a better solution.

@winksaville
Copy link
Contributor Author

@jamesrichford, thoughts on this?

@jamesadarich
Copy link
Member

Hey @winksaville,

This is interesting, I think the problem probably lies in that the execution is being attempted in a linear way where as the data it's recording is shallowly hierarchical. Perhaps instead we need to break these up into maps on this array where all the tests are already subdivided so we don't need to keep track of the current one as we've already filtered them correctly?

Perhaps this is too much work for this scope because I guess the focus of this is too get the tsc compiler flags in? If so then we may wanna go with something similar to this but maybe using null instead of undefined as a rule I think it's best whenever you've explicitly created something to use null instead of undefined - thoughts?

Maybe a potential middle ground is attempting to get these two current variables in every iteration instead and creating one if we didn't find a match. This means we can drop the previous item and make the code a bit more understandable?

@winksaville
Copy link
Contributor Author

Yes, I think you've probably identified the problem and yes beyond the scope. I'll change the undefined to null and see where that takes us.

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