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.

Sign up for GitHub

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

Jump to bottom

test: refactor predicate feature tests #1079

Merged
merged 36 commits into from
Jul 24, 2023
Merged

test: refactor predicate feature tests #1079

merged 36 commits into from
Jul 24, 2023

Conversation

danielbate
Copy link
Contributor

@danielbate danielbate commented Jun 30, 2023 •
edited
Loading

Bringing some of the testing standards from #1043 to the predicate package, this PR does the following:

  • Moves all predicate based tests from the fuel-gauge package into the predicate package inside a feature tests directory
  • Refactored predicate based integration tests in fuel-gage
  • Moved the predicate unit tests also to the feature test directory in the predicate package
  • Moved the integration and feature tests into more logical file structure and grouped based on functionality
  • Refactored fixtures and types into appropriate directories
  • Refactored tests to DRY up the code and moved shared code to utils directories
  • Changed the language used in all test cases to follow the gherkin syntax and standardise

danielbate added 14 commits May 30, 2023 13:28
chore: fix spellings in typegen
04cf5a6
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
100b1f7
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
0bbdbb1
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
90b5d3c
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
047ef9b
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
fe875a2
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
b14ad1d
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
9a7cf0e
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
dc8461f
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
d822b51
Merge branch 'master' of https://github.com/FuelLabs/fuels-ts
55cc6c3
test: remove predicate tests from fuel gauge
919f390
test: move predicate feature tests to predicate package
62f9508
chore: linting
09433b7
@danielbate danielbate marked this pull request as draft June 30, 2023 10:07
@danielbate danielbate force-pushed the db/test/refactor-predicate-tests branch from 859e960 to 09433b7 Compare June 30, 2023 10:18
arboleya
arboleya reviewed Jul 5, 2023
View reviewed changes
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

It seems that a lot of stuff is showing up ad deleted/added when it should appear as "renamed". This makes it harder to review big PRs since one needs to consider everything as new additions and, therefore, read it all over again.

Check this comment from @camsjams for a similar case:

In short, you'd need to delete/add things on the same commit, so GIT understands it as a rename.

Could you please consider doing the same for this PR?

@danielbate
Copy link
Contributor Author

danielbate commented Jul 5, 2023 •
edited
Loading

@arboleya I definitely can merge the commits. I am conscious however this won't get the desired effect due to the changes within the files, and that I have split 2 files into 8.

@arboleya
Copy link
Member

arboleya commented Jul 5, 2023

Oh, now I see. You're right; no worries then. Thanks!

The review should take a little longer, but nothing critical.

camsjams
camsjams suggested changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@camsjams camsjams left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I do like the organization, but this goes against previous decisions that aim to help us better align tests with the Rust SDK, see:
#492
#526

Fuel-gauge gives us some great benefits:

  • all contract related testing inside the same place
  • tests based on the fuels package export as a whole are more valuable
  • easier to copy over rust sdk Sway tests
  • easier to spin up new Sway tests

I think the spirit of having contract related testing inside the fuel-gauge should continue

The problem with predicates is that Sway programs don't work without Abi Coder and Script Request packages, so to move the Sway programs inside of the predicate package still violates some of these boundaries we are aiming to solve.

@danielbate danielbate marked this pull request as draft July 13, 2023 18:38
@danielbate
Copy link
Contributor Author

Thankyou for the fuel-gauge insight @camsjams . As discussed over call with the @FuelLabs/sdk-ts team, fuel-gauge will remain the source for the integration tests. Therefore I will move the sway tests back into there, but maintaining the seperation of tests that this PR brings. With the view of bringing that to more areas of fuel-gauge.

1 similar comment
@danielbate
Copy link
Contributor Author

Thankyou for the fuel-gauge insight @camsjams . As discussed over call with the @FuelLabs/sdk-ts team, fuel-gauge will remain the source for the integration tests. Therefore I will move the sway tests back into there, but maintaining the seperation of tests that this PR brings. With the view of bringing that to more areas of fuel-gauge.

@danielbate danielbate mentioned this pull request Jul 18, 2023
Merged
@danielbate danielbate marked this pull request as ready for review July 19, 2023 13:33
@danielbate
Copy link
Contributor Author

@FuelLabs/sdk-ts I've now made changes based off our previous conversation around keeping integration tests in fuel-gauge. I'll update the testing standards in #1043 once this has been merged.

camsjams
camsjams approved these changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@camsjams camsjams left a comment

Choose a reason for hiding this comment

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

Amazing!

@danielbate danielbate enabled auto-merge (squash) July 24, 2023 15:21
@danielbate danielbate merged commit 9da0e6a into master Jul 24, 2023
7 checks passed
@danielbate danielbate deleted the db/test/refactor-predicate-tests branch July 24, 2023 15:31
@github-actions github-actions bot mentioned this pull request Jul 24, 2023
Merged
nedsalk pushed a commit that referenced this pull request Jul 25, 2023
@danielbate @arboleya @nedsalk
test: refactor predicate feature tests (#1079)
3ec6af1 
* chore: fix spellings in typegen

* test: remove predicate tests from fuel gauge

* test: move predicate feature tests to predicate package

* chore: linting

* test: fix file path in predicate contract test

* chore: linting

* test: add more set data test cases

* test: add test case for simulating a transaction

* chore: changeset

* test: remove redundant fixture

* test: add script to build predicates

* test: add missing forc projects to predicate worksapce

* chore: linting

* fix: fix incorrect method name in predicagte test
'
'

* test: move predicate integration tests to fuel gauge

* core: regen lock file

* docs: update doc paths

* chore: delete redundant test file

* chore: regen lock file

* test: remove redundant predicate test

---------

Co-authored-by: danielbate <--global>
Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arboleya arboleya arboleya approved these changes

@camsjams camsjams camsjams approved these changes

@digorithm digorithm Awaiting requested review from digorithm digorithm is a code owner

@Torres-ssf Torres-ssf Awaiting requested review from Torres-ssf Torres-ssf is a code owner

@Dhaiwat10 Dhaiwat10 Awaiting requested review from Dhaiwat10 Dhaiwat10 is a code owner

@nedsalk nedsalk Awaiting requested review from nedsalk nedsalk is a code owner

Assignees

@danielbate danielbate

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@danielbate @arboleya @camsjams