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

Refactoring: Splitted test/validators.js to multiple subfiles #1793

Closed

Conversation

Marcholio
Copy link
Contributor

@Marcholio Marcholio commented Oct 15, 2021

Splitted the huge test/validators.js file into multiple subfiles, one per validator function. This makes it easier to read the test cases and maintain them as well. Did not change any test content, only moved existing cases to new files.

This will naturally cause conflicts with currently open PRs, but they should be easy to tackle, once this is mered.

Fixes: #1791

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #1793 (26eb5c5) into master (f055c11) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1793   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          102       102           
  Lines         2072      2072           
  Branches       472       472           
=========================================
  Hits          2072      2072           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f055c11...26eb5c5. Read the comment docs.

tux-tn
tux-tn previously approved these changes Oct 16, 2021
Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

Awesome work @Marcholio ! Thank you for your efforts ❤️

cc @ezkemboi @profnandaa any feedback about this? it will be easier to handle tests in the future but the only issue is that we will have planty of merge conflicts for the already open PRs.

@ezkemboi
Copy link
Member

Yes, we will have merge conflicts, but, I like the idea of splitting tests.

@Marcholio
Copy link
Contributor Author

Updated the PR with latest updates from master. This should probably be merged soon to avoid further conflicts.

@profnandaa
Copy link
Member

@Marcholio -- thanks for this. Kindly let's hold off this one first until we are done with the backlog for this next release and then this can be a good one to reconsider during the down-time.

@profnandaa profnandaa added the blocked For PRs that are blocked due to pending discussions, etc. label Oct 30, 2021
@Marcholio Marcholio force-pushed the refactor/validators-tests branch from 7f3e537 to f2f18e5 Compare October 30, 2021 09:56
@tux-tn
Copy link
Member

tux-tn commented Oct 30, 2021

@Marcholio since we are in the process of merging many pending PRs i suggest you wait for all of them to be merged before fixing conflicts. (Thank you again for your efforts 🎉 )

@tux-tn
Copy link
Member

tux-tn commented Nov 16, 2021

@Marcholio Most of the ready to land PRs have been merged. You can go ahead and fix conflicts/add missing tests.
Thank you !

@Marcholio Marcholio force-pushed the refactor/validators-tests branch from f2f18e5 to 26eb5c5 Compare November 17, 2021 14:36
@Marcholio
Copy link
Contributor Author

@tux-tn Rebased & solved the conflicts now

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

Thank you @Marcholio !

@profnandaa Can we merge this PR?

@WikiRik
Copy link
Member

WikiRik commented Aug 19, 2022

@rubiin Do you think we can rebase this and merge it soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked For PRs that are blocked due to pending discussions, etc. 🎉 first-pr maintenance needs-more-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split test/validators.js file to separate sub files
5 participants