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

Fix RocksDb issue, ensure rocksdb runs in CI #199

Merged
merged 12 commits into from
Mar 5, 2022
Merged

Fix RocksDb issue, ensure rocksdb runs in CI #199

merged 12 commits into from
Mar 5, 2022

Conversation

Voxelot
Copy link
Member

@Voxelot Voxelot commented Mar 5, 2022

After adding a new column to the database, I forgot to increment the column number constant which was causing errors at runtime.

To avoid this from happening again, I made it so that the tests run twice, once with default features enabled (rocksdb) and again with default features disabled (in-memory).

However, due to the way our integ tests in fuel-gql-client were setup, it was causing a bug in the v2 feature resolver when attempting to use cargo test --no-default-features. Specifically, it seemed like the feature resolver was unifying dev-deps in the fuel-gql-client with the fuel-core crate in the workspace, causing fuel-core to always be compiled with default features enabled when using --no-default-features at the workspace level. In order to fix this and make it possible to run tests without default features enabled on fuel-core, I had to move all the integ tests into their own crate (with normal deps only). This is better in the long run and is more similar to how projects like diesel-rs are structured.

@Voxelot Voxelot requested a review from adlerjohn March 5, 2022 03:54
@Voxelot Voxelot mentioned this pull request Mar 5, 2022
3 tasks
Cargo.toml Outdated Show resolved Hide resolved
fuel-client/Cargo.toml Outdated Show resolved Hide resolved
fuel-tests/Cargo.toml Outdated Show resolved Hide resolved
fuel-tests/Cargo.toml Show resolved Hide resolved
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

LGTM, though I would prefer a second set of eyes (maybe @rakita or @AlicanC)

Copy link
Contributor

@AlicanC AlicanC left a comment

Choose a reason for hiding this comment

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

I upgraded fuels-ts, block-explorer and swayswap-demo from 0.3.2 to the head of this branch and all look fine.

@Voxelot Voxelot merged commit 1bdd972 into master Mar 5, 2022
@Voxelot Voxelot deleted the fix-rocks-db branch March 5, 2022 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants