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

Test workflow improvements #803

Merged
merged 4 commits into from
Jul 26, 2017
Merged

Test workflow improvements #803

merged 4 commits into from
Jul 26, 2017

Conversation

tilsammans
Copy link
Contributor

@tilsammans tilsammans commented Jul 25, 2017

With these changes, testing locally should become a lot more comfortable, especially on macOS.

  • document the Rubies we are using for tests
  • document the need for bundler gem
  • commit package-lock.json file for reproducible tests to be done separately
  • switch to Poltergeist for headless tests
  • test for existence of socket file
  • ease the requirement for tempfile location

- document the Rubies we are using for tests
- document the need for `bundler` gem
- commit `package-lock.json` file for reproducible tests
- switch to Poltergeist for headless tests
- test for existence of socket file
- ease the requirement for tempfile location
@tilsammans
Copy link
Contributor Author

I was done fighting to install Qt for the umpteenth time. It's so heavy a dependency, whereas Poltergeist simply provides the correct binaries. No more need to install the full Xcode app install either, saves about 5 GB!

I eased the strict test for a Tempfile path, as these are in /var on macOS and in general, this decision is for the OS, it cannot be relied upon in specs.

Unsure if we are to ignore the npm lockfile or include it; I included it here.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.059% when pulling 60b0f27 on scrollytelling:test-setup-improvements into 03dc82c on codevise:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.059% when pulling 60b0f27 on scrollytelling:test-setup-improvements into 03dc82c on codevise:master.

Copy link
Member

@tf tf left a comment

Choose a reason for hiding this comment

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

Great suggestions. Especially adding a package-lock.json is something I had been meaning to do. Which version of npm do you use? I upgraded to 5.3, generated a lock file myself and it turned out slightly different (including version properties with urls instead of version and resolved properties). Running npm install while your lock file is already present, removes fsevent (which I guess is this npm issue). So maybe let's split adding package-lock.json into a separate PR.

I'm fine with using Poltergeist, but it appears to output console.log by default in test results. Now there are a bunch bandwidth detection outputs in the logs. Not sure how bad this is or how easily it can be fixed.

socket: /var/run/mysqld/mysqld.sock
<% elsif File.socket?('/tmp/mysql.sock') %>
socket: /tmp/mysql.sock
<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest adding an else case here and failing with a helpful error message. Maybe we can use a YAML anchor to prevent duplication of these blocks since are starting to get more complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add the macro and fall back to localhost:3306, which should cover everything.

@tilsammans
Copy link
Contributor Author

tilsammans commented Jul 25, 2017 via email

@tilsammans
Copy link
Contributor Author

Which version of npm do you use?

I was on 5.1, which explains the difference in lock files. I will remove this bit from the pull request.

The format changed between npm 5.1 and 5.3.
We need to have a separate pull request to discuss the pros and cons. Maybe switch to yarn instead, if they have a stable lockfile format.
and add a final else that defaults to localhost:3306
@tilsammans
Copy link
Contributor Author

Now there are a bunch bandwidth detection outputs in the logs.

This is when you run the test locally and on Travis, right? I don't mind those, and the output doesn't seem to affect test results. I would say it can stay as is. (Maybe even move this output away from console.log and into the pageflow object).

Other changes done.

@coveralls
Copy link

coveralls commented Jul 25, 2017

Coverage Status

Coverage remained the same at 99.059% when pulling fa1a65c on scrollytelling:test-setup-improvements into 03dc82c on codevise:master.

@tilsammans
Copy link
Contributor Author

I am considering using PhantomJS 2.1.1 on Travis, what do you think? I am running 2.1.1 locally...

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.059% when pulling e2dd80f on scrollytelling:test-setup-improvements into 03dc82c on codevise:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.059% when pulling e2dd80f on scrollytelling:test-setup-improvements into 03dc82c on codevise:master.

@tf tf added this to the v12.1 milestone Jul 26, 2017
@tf tf requested a review from aviav July 26, 2017 08:55
@tf
Copy link
Member

tf commented Jul 26, 2017

@aviav can you make sure the tests still pass on your machine after these changes? I vaguely remember their was something special about your setup.

@aviav aviav self-assigned this Jul 26, 2017
@aviav
Copy link
Contributor

aviav commented Jul 26, 2017

Everything passing with flying colors, even some specs that were previously failing in my development. Of course that looked like it was related to some Qt-webkit stuff.

It also looks like I cannot set my review to 'approve' since @tf already approves, but I would approve if I could :-)

EDIT: Found it :-)

@aviav aviav removed their assignment Jul 26, 2017
@tf tf merged commit 7ff839b into codevise:master Jul 26, 2017
@tilsammans tilsammans deleted the test-setup-improvements branch July 26, 2017 15:26
@tilsammans tilsammans restored the test-setup-improvements branch July 26, 2017 20:18
tilsammans added a commit to scrollytelling/app that referenced this pull request Nov 12, 2017
2017-11-07

[Compare changes](codevise/pageflow@12-0-stable...v12.1.0)

- Include HLS in output presences of legacy files
  ([#872](codevise/pageflow#872),
   [#867](codevise/pageflow#867))

  The database migration for Pageflow 12.0 which updates output
  presences of existing video files is missing the HLS variant. This
  causes HLS urls of existing video files to be rendered as
  `undefined`.

  To apply the fix, install migrations and migrate your database. This
  fix has previously been released as part of version 12.0.2.

- Add Resque::Server to the generated routes
  ([#871](codevise/pageflow#871))

  Mounting the Resque web server makes it easier to inspect background
  workers and restart jobs that have failed. See the issue description
  of [#856](codevise/pageflow#856) on how to
  add this to existing apps.

- The theme configured on account level now only acts as a default for
  new entries. After enabling the `selectable_themes` feature, a theme
  selection dialog is available inside the editor from the "Title and
  Options > Appearance" tab. The dialog allows configuring the theme
  on a per revision basis.
  ([#781](codevise/pageflow#781),
   [#897](codevise/pageflow#897))

- Use page from url hash es landing page
  ([#832](codevise/pageflow#832))
- Do not record history when changing pages via scrolling
  ([#831](codevise/pageflow#831))
- Improve text tracks and info box display logic
  ([#826](codevise/pageflow#826))
- Bug fix: Fix order of public i18n fallback
  ([#883](codevise/pageflow#883))
- Bug fix: Prevent display of NaN duration in video controls
  ([#878](codevise/pageflow#878))
- Bug fix: Prevent 404 when share image has been deleted
  ([#816](codevise/pageflow#816))

- Use searchable select boxes in admin forms
  ([#888](codevise/pageflow#888))
- Remove sensitive data from active admin downloads
  ([#899](codevise/pageflow#899))
- Add config option to prevent multi account users
  ([#848](codevise/pageflow#848),
   [#868](codevise/pageflow#868))
- Add config options to restrict account manager permissions
  ([#849](codevise/pageflow#849))
- Fix N+1 query in account admin users tab
  ([#877](codevise/pageflow#877))
- Bug fix: Hide restore link and snapshot button for entry previewers
  ([#853](codevise/pageflow#853))
- Bug fix: Use copy of current_user in profile form
  ([#850](codevise/pageflow#850))
- Bug fix: Ensure new entry button is hidden for editors
  ([#847](codevise/pageflow#847))
- Bug fix: Show folder edit button only for publishers and above
  ([#838](codevise/pageflow#838))
- Bug fix: Allow :create entry only for publishers on accounts, not on entries
  ([#836](codevise/pageflow#836))

- Widget configuration
  ([#694](codevise/pageflow#694),
   [#809](codevise/pageflow#809))
- Bug fix: Ensure page order in editor preview stays up to date
  ([#898](codevise/pageflow#898))
- Bug fix: Switch off file meta data edit links when uploading
  ([#857](codevise/pageflow#857))
- Bug fix: Improve polling for HostedFile state
  ([#822](codevise/pageflow#822))
- Bug fix: Handle undefined page title.
  ([#763](codevise/pageflow#763))

- Use relative urls inside dash and hls playlists
  ([#842](codevise/pageflow#842))
- Use web audio api for volume fading if available
  ([#800](codevise/pageflow#800),
   [#863](codevise/pageflow#863))

- Add panorama_mask image file style
  ([#830](codevise/pageflow#830))
- Bug fix: Make url template generation more robust
  ([#876](codevise/pageflow#876))

- Themes can now be guarded by feature flags
  ([#765](codevise/pageflow#765))
- Extend EncodingConfirmation by public account attribute
  ([#817](codevise/pageflow#817))
- Extend query interface
  ([#815](codevise/pageflow#815))
- Accept options for accounts admin menu via config
  ([#811](codevise/pageflow#811))
- Add placeholder options to textareainputview
  ([#807](codevise/pageflow#807))
- Call hook on entry publication
  ([#806](codevise/pageflow#806))
- Add rake task and resque job to delete old auto snapshots
  ([#861](codevise/pageflow#861),
   [#882](codevise/pageflow#882))
- Generate a secure password in the seeds
  ([#775](codevise/pageflow#775))

- Allow specifying opacity of image variant logo
  ([#799](codevise/pageflow#799))
- Allow setting size of custom loading spinner background
  ([#798](codevise/pageflow#798))
- Add theme option to right align logo in desktop layout
  ([#797](codevise/pageflow#797))
- Add theme option to hide scroll indicator
  ([#790](codevise/pageflow#790))
- Make content text max width configurable
  ([#780](codevise/pageflow#780),
   [#804](codevise/pageflow#804))

- Import nginx-upload-module guide from wiki
  ([#814](codevise/pageflow#814),
   [#821](codevise/pageflow#821))
- Add documentation for versioning policy
  ([#866](codevise/pageflow#866))
- Fix small typos
  ([#787](codevise/pageflow#787))

- Precompile assets in Travis run
  ([#873](codevise/pageflow#873))
- Test workflow improvements
  ([#803](codevise/pageflow#803))
- Generate admins with account membership in specs
  ([#840](codevise/pageflow#840))
- Use rails 4.2.9 in tests
  ([#837](codevise/pageflow#837))
- Clean up memberships admin code
  ([#835](codevise/pageflow#835))
- Basic tests for UsersTab add user button
  ([#805](codevise/pageflow#805))
- Use codevise/teaspoon for logging backtraces on console
  ([#786](codevise/pageflow#786))
- Split vendored code from our own
  ([#885](codevise/pageflow#885))
- Introduce ApplicationRecord
  ([#889](codevise/pageflow#889))
- Move configuration into a concern
  ([#794](codevise/pageflow#794))
- Read the attribute instead of super
  ([#810](codevise/pageflow#810))
- Destroy widgets when parent subject is destroyed
  ([#834](codevise/pageflow#834))
- Fix dummy app generation on Travis for Rubygems 2.7
  ([#893](codevise/pageflow#893))
- Update contents of gemspec
  ([#891](codevise/pageflow#891))
- Bug fix: Work around breaking change in resque_mailer 2.4.3
  ([#894](codevise/pageflow#894))

See
[12-0-stable](https://github.com/codevise/pageflow/blob/12-0-stable/CHANGELOG.md)
branch for previous changes.
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.

4 participants