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

Test cleanup #437

Merged
merged 4 commits into from Feb 12, 2019
Merged

Test cleanup #437

merged 4 commits into from Feb 12, 2019

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Feb 11, 2019

This half-cleaned tests directory together with the outdated runAll() test run function in tester.js is for quite some time some reoccurring source of confusion, latest during the work on #406.

This PR does some cleanup here:

Separate manual directory with the manual constantionpleSstoreTest test file

This was once written by Vinay since the official test suite was not yet available. Tests still pass so would be a bit a pity to throw away. For the current situation this extra manual command might be a bit too much. Reasoning would be nevertheless that there is then a place for similar tests in the future if necessary and things have some place then and there will be some higher chance that structure won't get bloated on stuff like this again.

Update: Tests deleted after discussion.

Included hooked.js into the API test suite

These tests are still working. We can remove/adopt depending on what we do with this part of the API. For now the code is still there so this fits better in the API directory than having some (too prominent) separate place.

Integrated genesis hash test into the API test suite

This test was also floating around, not used for quite some time but still working. Have adopted and added to the API test suite. Genesis functionality of StateManager is also not sure to be kept on final extracted version. Nevertheless for the moment it's still there and used.

Replaced outdated runAll() test run code with info message

When npm run test is run this is now showing a Command-not-used message. The code within the function runAll() is commented out. This was/is some major source of confusion for newcomers who tend to integrate their new tests there.

Update: Function deleted after discussion.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Thanks for this, I'm totally on-board with cleaning the tests.

package.json Outdated Show resolved Hide resolved
tests/tester.js Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 11, 2019

Coverage Status

Coverage decreased (-1.9%) to 89.158% when pulling ed5041f on test-cleanup into 57293c6 on master.

@holgerd77
Copy link
Member Author

Just x-checked, the decrease in test coverage is from the lib/hooked.js file now included in the line count, so with this PR relevant lines covered increases but also the total line count (see PR build vs master build.

So no actual coverage decrease and we can merge if everything else is ok.

s1na
s1na previously approved these changes Feb 11, 2019
@holgerd77
Copy link
Member Author

Already reviewed by @s1na, no production code touched, will merge.

@holgerd77 holgerd77 merged commit 1e6c92d into master Feb 12, 2019
@holgerd77 holgerd77 deleted the test-cleanup branch February 12, 2019 10:20
@holgerd77 holgerd77 mentioned this pull request Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants