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

Duplicate provider tests #1104

Open
3 of 4 tasks
austinabell opened this issue Apr 5, 2023 · 4 comments
Open
3 of 4 tasks

Duplicate provider tests #1104

austinabell opened this issue Apr 5, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@austinabell
Copy link
Contributor

Prerequisites

  • I'm using the latest version of near-api-js.
  • I have tried to start with a fresh project and reproduce the defect with minimal code changes.
  • I have read the console error messages carefully (if applicable).

Description

There are three of the same provider tests in providers, accounts, and near-api-js packages. Diffing the files, it seems like the near-api-js tests are a superset of the other two, but I'm not sure what reason there would be to have the duplication.

Because of this, I added my new test in the near-api-js package in #1103, but I wasn't sure if I should delete the other two files or duplicate this test to those.

Reproducible demo

No response

Steps to reproduce

n/a

Expected behavior

n/a

Actual behavior

n/a

Your environment

  • NEAR JavaScript API version used:
  • Frontend framework (if applicable):
  • Relevant dependencies (if applicable):

Self-service

  • I'd be willing to fix this bug myself.
@austinabell austinabell added the bug Something isn't working label Apr 5, 2023
@andy-haynes
Copy link
Collaborator

andy-haynes commented Apr 5, 2023

Hey @austinabell, this was an intentional (albeit temporary) change as part of breaking near-api-js into packages.

The providers tests in packages/near-api-js are meant only as a temporary measure to ensure near-api-js tests continued to pass during this transition. Once we're ready to deprecate near-api-js in favor of the new packages, we can confidently remove those tests (and parallelize tests, but that's a different story).

Across accounts and providers there should be no overlap. They should all be in providers but many of the tests took advantage of helper methods referencing the accounts package, which already references providers. My intention was that these tests would be refactored to lose the dependency on accounts and moved back to the providers package. Given the size of the refactor I refrained from modifying any code unnecessarily so they remain in this indeterminate state at the moment.

I'm keen to hear your thoughts though, I meant to make a note of this in the refactor PR but it slipped my mind. IIRC most of the other tests got to remain intact.

@andy-haynes
Copy link
Collaborator

andy-haynes commented Apr 5, 2023

I wasn't sure if I should delete the other two files or duplicate this test to those.

Putting in providers/test/providers.test.js makes the most sense to me 👍 I'll take a look at the rest of that PR today

@austinabell
Copy link
Contributor Author

austinabell commented Apr 5, 2023

I wasn't sure if I should delete the other two files or duplicate this test to those.

Putting in providers.test.js makes the most sense to me 👍 I'll take a look at the rest of that PR today

They're all named providers.test.js :D I'm assuming you mean the one in the near-api-js package

I'm keen to hear your thoughts though, I meant to make a note of this in the refactor PR but it slipped my mind. IIRC most of the other tests got to remain intact.

I don't have an opinion, and this doesn't block anything I'm doing, just wanted to make sure I'm putting tests in an acceptable place and to mention it in case it wasn't intentional

@andy-haynes
Copy link
Collaborator

They're all named providers.test.js :D I'm assuming you mean the one in the near-api-js package

Just my intermittent illiteracy kicking in, I meant providers/test and thought that's where I had seen it in your PR 😅

New tests for Providers would go in @near-js/providers. If it needs to be exposed in near-api-js (as opposed to just @near-js/providers) for some reason, then it would make sense to duplicate the tests there as well. Hopefully I'll have some guidance on the future near-api-js soon and will be able to clean this up some.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants