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

Revert "Revert #334 - Themes localization" #390

Closed
wants to merge 3 commits into from

Conversation

jagthedrummer
Copy link
Contributor

@jagthedrummer jagthedrummer commented Aug 9, 2023

Unfortunately we had to revert #334 because it had broken the date and date/time pickers. After merging #334 a lot of stuff changed so I couldn't generate a clean revert PR. So #389 was a "fresh" PR that was a commit-by-commit revert of each of the commits in #334.

This PR is an auto-generated revert of #389. Which effectively means it's a revert of the revert of #334. AKA it reintroduces the changes from #334 and brings back the theme localization that was done in that PR.

I'm just opening this so that we don't lose track of it, and so there's a place to pick back up if/when someone has the time.

Reverts #389

@jagthedrummer
Copy link
Contributor Author

jagthedrummer commented Aug 9, 2023

Strangely, all tests are passing in this repo, but not in the starter repo when linked to this same branch. I suspect that we're not linking everything in the core repo to the starter repo when we check it out in CI.

https://app.circleci.com/pipelines/github/bullet-train-co/bullet_train/1982/workflows/fd4ed8da-6526-463c-ab5b-c15dc0b3d522

@jagthedrummer
Copy link
Contributor Author

It turns out that the test discrepancy was because we just weren't running as many tests here as were are in the stater repo. I added the missing tests to the circle config, and now this PR is showing the test failures.

@gazayas
Copy link
Contributor

gazayas commented Aug 10, 2023

I found that the changes I added here bullet-train-co/bullet_train@96fb534 and here bullet-train-co/bullet_train@b411a21 fixed these tests.

I took these from bullet-train-co/bullet_train#811 which is also related.

image

@3pns
Copy link
Contributor

3pns commented Aug 10, 2023

I started investigating the new test failure by recovering my PR here #393

I verified manually that the scaffolding system still works both with the latest version of the starter repo and on our side as well.

I see that the tests pass on master, I can only think of 2 possible scenarios :

  • we may not link everything properly in the core repo to the starter repo as you suggested @jagthedrummer . I have seen the pickers not load when I have done this when doing my tests locally.
  • my changes may prevent the daterangepicker to load during system tests ? This is the reason I used the trick that @gazayas pointed out in Add tests for localized date and datetime fields bullet_train#811 . However it could be possible that the pickers don't load in my dev environment tests but load successfully on CI tests.

Maybe is there something else I did not think about ?

@jagthedrummer
Copy link
Contributor Author

Hey @3pns, thanks for picking this back up! You can see the tests in question failing on #393. Here's a link directly to CI: https://app.circleci.com/pipelines/github/3pns/bullet_train-core/6/workflows/a37eff84-3695-48c9-92bf-605ef201b98f/jobs/20

Note that these are in test/system/super_scaffolding_partial_test.rb which is not run as part of the regular system tests.

There are instruction included in the top of that test file for how to run them locally. Previously when I had linked every gem that's in core (including ones that aren't explicitly listed in Gemfile but that are included in Gemfile.lock) to my local starter repo that test would fail for me in the same way that it fails in CI.

@jagthedrummer
Copy link
Contributor Author

Closing this in favor of #393.

@gazayas
Copy link
Contributor

gazayas commented Aug 10, 2023

@3pns

we may not link everything properly in the core repo to the starter repo as you suggested @jagthedrummer . I have seen the pickers not load when I have done this when doing my tests locally.

☝️

Excited for #396! It's been a long time coming.

@jagthedrummer
Copy link
Contributor Author

Reopening this temporarily to see what effect #396 has on testing this branch.

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