Skip to content

Conversation

aldesantis
Copy link
Member

@aldesantis aldesantis commented Dec 13, 2019

This PR contains two fixes which allow us to run the tests for solidus_support both locally and in CI.

ActiveRecord adapters

Fixes the gem for AR adapters not being loaded.

Sprockets

Fixes an issue where Sprockets complains that there's no manifest file by locking to Sprockets 3.

Unfortunately, Sprockets 4, which introduced manifests, doesn't support specifying a custom path for the assets manifest, which obviously doesn't work with in-memory apps.

@kennyadsl submitted a PR which would allow Sprockets to support custom manifest paths, but it was not merged yet.

@aldesantis aldesantis requested a review from a team December 13, 2019 15:57
@aldesantis aldesantis self-assigned this Dec 13, 2019
@kennyadsl
Copy link
Member

@aldesantis can we rebase this one please?

There is an issue with Sprockets 4 not accepting a custom path for
the assets manifest, which doesn't play well with in-memory dummy
apps such as the one we use in this gem.

A fix was provided for sprockets-rails[1] but it was not accepted
yet.

[1]: rails/sprockets-rails#446
@aldesantis aldesantis changed the title Fix Sprockets complaining about the lack of a manifest Fix issues with test runs Dec 14, 2019
@aldesantis
Copy link
Member Author

@kennyadsl done, and also included a fix for AR adapters to allow CircleCI to run with both PostgreSQL and MySQL.

I was a bit torn here, because this gem doesn't really need to run with them and could just use in-memory SQLite, but I figured it's better to have the entire ecosystem run tests with the same stack for consistency. At some point in the future we may introduce DB-dependent changes and we don't want to rely on our memory to remember to change the CI configuration then.

@aldesantis aldesantis merged commit 54cdba6 into solidusio:master Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants