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

Actually get dummy test app running properly and passing feature tests #407

Merged
merged 5 commits into from
Jun 27, 2022

Conversation

jrochkind
Copy link
Contributor

@jrochkind jrochkind commented Jun 16, 2022

  • Configure dummy test all with the test controller/views for demo under test

  • properly set up sprockets for bootstrap 4 to include JS in dummy app

  • properly get all dummy app dependencies set up -- this is the weirdest most confusing part

    • dependencies are actually listed in .gemspec as development dependencies
    • but may also need to be require'd in application.rb, which would normally happen automatically if they were
      in a real Gemfile
  • We give up on being able to test both bootstrap 3 and 4 -- that's not really tenable with the dummy app approach, since they use different gems entirely, and different local CSS files. We only test bootstrap 4 now.

  • Actually enable the formerly disabled feature tests!


Includes in this PR one commit:

FileSystem driver has new allow_relative_home param

If set to true, then home can be a relative path (relative to Rails.root).

The use case is mainly for CI/dummy test app setup. Added new param to make it opt-in, to make sure it's completely backwards compat and does not introduce any potential security issues -- you need to opt in to new supported behavior with new allow_relative_home param.

@jrochkind jrochkind force-pushed the feature_tests_on_dummy branch 2 times, most recently from deacf46 to febaa8d Compare June 16, 2022 18:10
* Configure dummy test all with the test controller/views for demo under test
* properly set up sprockets for bootstrap 4 to include JS in dummy app
* properly get all dummy app dependencies set up -- this is the weirdest most confusing part
  * dependencies are actually listed in .gemspec as development dependencies
  * but may also need to be require'd in application.rb, which would normally happen automatically if they were
    in a real Gemfile

* We give up on being able to test both bootstrap 3 and 4 -- that's not really tenable with the dummy app approach, since they use different gems entirely, and different local CSS files. We only test bootstrap 4 now.

* Actually enable the formerly disabled feature tests!
@jrochkind jrochkind force-pushed the feature_tests_on_dummy branch from febaa8d to d7db9f6 Compare June 16, 2022 18:11
@jrochkind
Copy link
Contributor Author

Ok getting very close!

But we were trying to configure browse_everything_providers.yml, under test, with a file_system provider pointing to test app source.

But it can only take an absolute path? And we need to commit to repo... where the absolute path is going to be different depending on where the repo is checked out, etc.

Hmm. We may expand it to take a relative path (to CWD)... if opted into?

If set to true, then `home` can be a relative path (relative to Rails.root).

The use case is mainly for CI/dummy test app setup. Added new param to make it opt-in, to make sure it's completely backwards compat and does not introduce any potential security issues -- you need to opt in to new supported behavior with new allow_relative_home param.
@jrochkind jrochkind force-pushed the feature_tests_on_dummy branch from b86417d to c03a18a Compare June 16, 2022 20:22
Base automatically changed from fixed_dummy_test_app to main June 22, 2022 17:30
…d since we've switched to dummy_test_app that has versions of these files committed
@jrochkind jrochkind marked this pull request as ready for review June 22, 2022 17:41
@jrochkind
Copy link
Contributor Author

OK building on top of #406 this is now ready to review -- feature tests re-enabled and passing, on our new checked-in dummy app!

@@ -2,6 +2,7 @@
version: 2.1
orbs:
samvera: samvera/circleci-orb@1.0
browser-tools: circleci/browser-tools@1.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -19,6 +20,8 @@ jobs:
environment:
RAILS_VERSION: << parameters.rails_version >>
steps:
- browser-tools/install-chrome
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -3,7 +3,8 @@
# The file_system provider can be a path to any directory on the server where your application is running.
#
file_system:
home: /Users/jrochkind/code/browse-everything/.internal_test_app
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am sorry for missing this line within a previous pull request review.

@@ -7,7 +7,7 @@

shared_examples 'browseable files' do
# This is a work-around until the support for Webpacker is resolved
xit 'selects files from the filesystem' do
it 'selects files from the filesystem' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -1,7 +1,7 @@
# frozen_string_literal: true

describe 'Compiling the stylesheets', type: :feature, js: true do
xit 'does not raise errors' do
it 'does not raise errors' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@jrgriffiniii jrgriffiniii left a comment

Choose a reason for hiding this comment

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

This is phenomenal, thank you very much again for undertaking this much needed improvement!

@jrgriffiniii jrgriffiniii merged commit e194d2a into main Jun 27, 2022
@jrgriffiniii jrgriffiniii deleted the feature_tests_on_dummy branch June 27, 2022 18:08
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.

2 participants