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/activestorage dependency details #1480

Merged

Conversation

MrJoy
Copy link
Contributor

@MrJoy MrJoy commented Dec 4, 2022

Description

Minor fixups for noise when ActiveStorage isn't present, and for workstation setup for contributors.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Manual review steps

Executed fragment of code in IRB.

Unfortunately, I'm encountering major problems getting the test suite running locally.

For starters, seemingly every test fails because of a license key check where the license_key value is nil. If I change the stub, then I get other problems:

  • Missing URL helpers in a bunch of tests (undefined method sign_out_path_name' for #Module:0x000000010eff3598`)
  • Failures in the code that's meant to check the licensing functionality and is rendered incorrect by having changed the stub
  • Capybara failures because the user wasn't logged in and so the login page was rendered (e.g. Unable to find css "turbo-frame[id='dashy_users_metric'][data-card-index='3']" -- screenshot shows a completely unstyled login page), etc.

Basically, just all kinds of weirdness.

@codeclimate
Copy link

codeclimate bot commented Dec 4, 2022

Code Climate has analyzed commit 91e33fc and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@adrianthedev
Copy link
Collaborator

Thanks for writing and for the improvements @MrJoy.

Regarding the test runs... I pushed some changes to this PR.
Can you pleasepull these latest changes and copy over the .env.test file (cp spec/dummy/.env.test.sample spec/dummy/.env.test) to see if you can run the tests?

@MrJoy
Copy link
Contributor Author

MrJoy commented Dec 5, 2022

Much improved! I initially had some failures, but that was due to my hack for the license request stub in place. Removing that, and rerunning the failed tests, they all passed. I'm doing a full run again just to be sure, but so far it looks like things work.

@MrJoy
Copy link
Contributor Author

MrJoy commented Dec 5, 2022

Ah. Not quite. A couple errors turned up again on the full run:

image

Also, the Unable to find css "turbo-frame[id='dashy_users_metric'][data-card-index='0']" turned up again. Guessing there's some intermittency going on.

@MrJoy
Copy link
Contributor Author

MrJoy commented Dec 5, 2022

Ok, no, they're failing consistently. I must've not re-run all the failed tests.

rspec ./spec/system/avo/dashboards_spec.rb:54 # Dashboards dashboard with cards metric card
rspec ./spec/system/avo/dashboards_spec.rb:127 # Dashboards dashboard with cards chartkick area card

The exact specs that are failing.

@adrianthedev
Copy link
Collaborator

Yes, it's a race condition that happens when the cards load up in the dashboard. There are about 14 requests being made in the same time (for each card) and that's where it the race condition manifests itself. Not sure why it only happens on local (the CI passes everytime). Probably the hardware executes at a different speed.

We worked on and fixed this issue in Avo 3.

I think I'd merge this PR as is so we don't pollute it with a bunch of other unrelated changes and come back with another PR to fix that in Avo 2.

WDYT?

@adrianthedev adrianthedev mentioned this pull request Dec 7, 2022
4 tasks
@MrJoy
Copy link
Contributor Author

MrJoy commented Dec 7, 2022

I'm completely fine with merging the PR if you are.

@adrianthedev adrianthedev merged commit 54efb81 into avo-hq:main Dec 8, 2022
@MrJoy MrJoy deleted the fix/activestorage-dependency-details branch December 8, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants