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

Consolidate utility testing functions where possible #832

Closed
Tracked by #1256
evan-forbes opened this issue Oct 3, 2022 · 6 comments
Closed
Tracked by #1256

Consolidate utility testing functions where possible #832

evan-forbes opened this issue Oct 3, 2022 · 6 comments
Labels
good first issue Good for newcomers testing items that are strictly related to adding or extending test coverage

Comments

@evan-forbes
Copy link
Member

As we've discussed synchronously many times, there's quite a bit of redundant testing utility functions. We should consolidate where possible. If its not possible due to cyclic imports, then we should consider putting those tests in their own package.

related to #829

@evan-forbes evan-forbes added testing items that are strictly related to adding or extending test coverage good first issue Good for newcomers labels Oct 3, 2022
@evan-forbes evan-forbes moved this to TODO in Celestia Node Oct 3, 2022
@amoghrajesh
Copy link

@evan-forbes I can work on this issue. Can you point me to the files to get started?

@rootulp
Copy link
Collaborator

rootulp commented Oct 4, 2022

A candidate sub-task for this issue is to delete the duplicate definitions of randomValidNamespace:

  1. func randomValidNamespace() namespace.ID {
  2. func randomValidNamespace() namespace.ID {
  3. func randomValidNamespace() namespace.ID {
  4. func randomValidNamespace() namespace.ID {
  5. func RandomValidNamespace() namespace.ID {

The last function definition is the most preferred because it accounts for the parity and tail padding namespaces. Note: the duplicate functions are defined in different packages so the last function may need to be extracted to a package that can be imported by all usages of the duplicate definitions.

@amoghrajesh
Copy link

@rootulp @evan-forbes can we define a scope for this issue? We can not achieve all the changes in one PR. If we can scope them, it would be easier to work on

@rootulp
Copy link
Collaborator

rootulp commented Oct 4, 2022

Spun out #836 in-case @evan-forbes has other ideas for what this issue should include.

@evan-forbes
Copy link
Member Author

another instance per this #1991 (comment)

@rootulp
Copy link
Collaborator

rootulp commented May 16, 2024

Closing this b/c it's not actionable. Please re-open or create a new issue with more specific examples of the redundant testing utilities that need consolidation.

@rootulp rootulp closed this as not planned Won't fix, can't repro, duplicate, stale May 16, 2024
@github-project-automation github-project-automation bot moved this from TODO to Done in Celestia Node May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers testing items that are strictly related to adding or extending test coverage
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants