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

Remove test cases that vary based on codec for extendeddatacrossword_test.go #216

Closed
rootulp opened this issue Jul 5, 2023 · 2 comments · Fixed by #274
Closed

Remove test cases that vary based on codec for extendeddatacrossword_test.go #216

rootulp opened this issue Jul 5, 2023 · 2 comments · Fixed by #274
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@rootulp
Copy link
Collaborator

rootulp commented Jul 5, 2023

Context

The tests in extendeddatacrossword_test.go contain test cases for multiple codecs but after #151 this repo only supports one codec.

Example:

tests := []struct {
name string
// Size of each share, in bytes
shareSize int
codec Codec
}{
{"leopard", bufferSize, NewLeoRSCodec()},
}

Proposal

Refactor the tests in this file to remove test cases that differ based on codec. This will likely remove a level of indentation from the interesting parts of this file.

@staheri14
Copy link
Contributor

staheri14 commented Dec 11, 2023

Thanks for opening this issue!
Is there a long-term plan to exclusively use one codec? If so, it may be beneficial to refactor functions that currently accept a codec as an input parameter. These parameters would become redundant with only one supported codec. Alternatively, if multiple codecs will continue to be supported, it's useful to maintain the current structure of the tests. These tests comprehensively account for all input parameters of the function they aim to test, including the codec. (it is conceivable that if other codecs get added we need to re-deploy the same test structure as we currently have, resulting in redundant work). WDYT? @rootulp

@rootulp
Copy link
Collaborator Author

rootulp commented Dec 11, 2023

I don't see any GH issues in this repo for adding new codecs. I think the current test structure is a byproduct of development (i.e. historically multiple codecs were supported) so IMO we should refactor to just support one codec.

If we need to add a new codec in the future, the PR that adds a new codec can (and should) add test coverage for the new codec.

rootulp pushed a commit that referenced this issue Dec 12, 2023
<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview

Closed: [#216](#216)

<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 
-->

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
0xchainlover pushed a commit to celestia-org/rsmt2d that referenced this issue Aug 1, 2024
<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview

Closed: [#216](celestiaorg/rsmt2d#216)

<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 
-->

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants