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

Run all tests in CI #336

Closed
wants to merge 5 commits into from
Closed

Run all tests in CI #336

wants to merge 5 commits into from

Conversation

PragTob
Copy link
Collaborator

@PragTob PragTob commented Jan 27, 2020

Issue #332: CI allegedly isn't running all spec files

Changes

  • take the other spec files that aren't executed into account

Notes

  • should imo be revisited after move dummy project out of spec folder #333 if it is resolved
  • as one of these tests fails locally this will likely break on CI, it's intended to see that it does and that the number of tests actually increases - after that is done I'll add in another commit hopefully fixing the test

* Fixes #332
* should imo be revisited after #333 if it is resolved
* as one of these tests fails locally this will likely break on
  CI, it's intended to see that it does and that the number of
  tests actually increases
Log was complaining about it not being up to date, it's reasonable
to assume circleci executes all commands from project root
(I was wondering before how the consequent commands would work
if the `cd` effect stuck, this explains it)
@PragTob
Copy link
Collaborator Author

PragTob commented Jan 27, 2020

239 examples, 9 failures, 12 pending vs 227 examples, 0 failures, 12 pending

definitely executes more tests 💃

@PragTob
Copy link
Collaborator Author

PragTob commented Jan 27, 2020

And we fail to install node-sass on CircleCI which indicates we might need to install packages or tinker with the base image 😢

Full thing: https://circleci.com/gh/matestack/matestack-ui-core/660?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

yarn install v1.21.1
[1/4] Resolving packages...
[2/4] Fetching packages...
warning [email protected]: Invalid bin entry for "sha.js" (in "sha.js").
info [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
warning "@rails/webpacker > [email protected]" has unmet peer dependency "caniuse-lite@^1.0.30000697".
warning " > [email protected]" has unmet peer dependency "webpack@^4.0.0".
warning "webpack-dev-server > [email protected]" has unmet peer dependency "webpack@^4.0.0".
[4/4] Building fresh packages...
error /home/circleci/repo/spec/dummy/node_modules/node-sass: Command failed.
Exit code: 1
Command: node scripts/build.js
Arguments: 
Directory: /home/circleci/repo/spec/dummy/node_modules/node-sass
Output:
Building: /usr/local/bin/node /home/circleci/repo/spec/dummy/node_modules/node-gyp/bin/node-gyp.js rebuild --verbose --libsass_ext= --libsass_cflags= --libsass_ldflags= --libsass_library=
gyp info it worked if it ends with ok
gyp verb cli [
gyp verb cli   '/usr/local/bin/node',
gyp verb cli   '/home/circleci/repo/spec/dummy/node_modules/node-gyp/bin/node-gyp.js',
gyp verb cli   'rebuild',
gyp verb cli   '--verbose',
gyp verb cli   '--libsass_ext=',
gyp verb cli   '--libsass_cflags=',
gyp verb cli   '--libsass_ldflags=',
gyp verb cli   '--libsass_library='
gyp verb cli ]

It might be some combination of node-sass not being compatible with the node version 12 in the image I'll give updating it a try: sass/node-sass#2648

Just yarn upgrade node-sass didn't work so let's just get all
the new stuff - what could possibly go wrong, hey?
@PragTob
Copy link
Collaborator Author

PragTob commented Jan 27, 2020

And now lots of tests fail. The one in lib that I expected to fail but also tests in usage, some of them with a curios superclass mismatch error:

  8) Transition Component Example 1 - Perform transition from one page to another without page reload if related to app
     Failure/Error:
           class ExampleAppPagesController < ExampleController
             include Matestack::Ui::Core::ApplicationHelper
       
             def page1
               responder_for(Pages::ExampleApp::ExamplePage)
             end
       
             def page2
               responder_for(Pages::ExampleApp::SecondExamplePage)
             end
     
     TypeError:
       superclass mismatch for class ExampleAppPagesController
     # ./spec/usage/components/transition_spec.rb:62:in `block (2 levels) in <top (required)>'

`inject_into_file` only works when a file is actually present,
the README seems to be generated nowhere so it seems to assume
that it is manually created - hence manually created it in the
dummy app to make the test run pass.

Probably not the _best_ solution as the generator seems to mainly
be used in the core repo itself for which the dummy app doesn't
qualify. Hence working with a different example app/folder might
be the preferred solution.
@@ -0,0 +1,8 @@
# Core Components

This file exists here currently just so that the tests for `Matestack::Core::Generators::ComponentGenerator` can run as they expect such a file to exist.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jonasjabari this is a ... very pragmatic solution but potentially not the best one. Please check the commit message :)

@PragTob
Copy link
Collaborator Author

PragTob commented Jan 27, 2020

Super class failures + 1 other remain.

I have an idea about the super class failures, not entirely sure how to best fix it though - probably a more pragmatic fix than the right fix(tm) if it is what I believe it is (reopening classes all the time). Curios that it doesn't happen locally.

@PragTob
Copy link
Collaborator Author

PragTob commented Jan 28, 2020

Well, that didn't work :(

@PragTob
Copy link
Collaborator Author

PragTob commented Jan 28, 2020

Okay, so the problem is many fold:

  • classes are reopened all the time around the test suite changing their behaviour, as far as I've seen they always had the same super class so not entirely sure where the superclass type error comes from other than maybe it resolving to another one/a modified one so an easy fix could have been to rename them but then we get into problems with the routes (most proper way would probably to use anonymous classes but I'm unsure how to make them work with the routes)
  • the routes are actually global state so changing one thing to draw or what not will/can potentially break other tests

So it's more involved to solve this with different degrees of involvement:

  • of course we could continue the routes appending with custom routes but that seemed to get more involved changing these tests that I don't know yet
  • an alternative would also be that every test basically defined its own mini rails app instead of relying on one that seems to be shared
  • another alternative would be to just define all of these in the dummy app, so not dynamically and test against that which would also be more involved of course
  • namespace the custom classes by test could also be a solution to the name clashes that are initially causing this

Naturally there will be other solutions that I don't come up with yet :)

I'm also not sure why this doesn't happen locally, my easiest guess is that it's related to the parallel test running in CircleCI so deactivating that might also be an option while not ideal.

@jonasjabari
Copy link
Member

@PragTob thanks for the work here so far. I'm closing this as discussed. #368 is solving this topic for now. but we have to come back at your ideas on how to refactor the specs soon.

@jonasjabari jonasjabari added this to the 0.7.4 milestone Feb 10, 2020
@pascalwengerter pascalwengerter deleted the fix-run-all-tests-in-ci branch March 11, 2020 10:43
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