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

Sort files to require for coverage reports #14128

Merged

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Mar 2, 2017

Dir.glob don't guarantee the order of the files it returns
For example we have model in
app/model/rss_feed.rb and his extension in
app/model/rss_feed/import_export.rb - there is class
defined without inheriting from ApplicationRecord
so when class from import_export.rb is loaded first
is it loaded as RssFeed < Object and when
after this is app/model/rss_feed.rb loaded
it is causing error(this warning is suppressed by
silence_warnings in coverage_helper):

TypeError: superclass mismatch for class RssFeed

and then RssFeed < ApplicationRecord is not loaded

Sorting Dir.glob guarantee order so that
files will be first then subdirectories,..
() because '.'.ord < '/'.ord
(sort is also used in eager_load!
https://github.com/rails/rails/blob/master/railties/lib/rails/engine.rb#L475)

this sporadic failure can be seen here: https://travis-ci.org/ManageIQ/manageiq/jobs/206552109
(there are some my debug messages)

This PR is blocking #14041

thanks @isimluk for help!

cc @kbrock @jrafanie
@miq-bot assign @gtanzillo

@miq-bot add_label bug/sporadic test failure, test

Dir.glob don't guarantee the order of the files it returns
For example we have model in
app/model/rss_feed.rb and his extension in
app/model/rss_feed/import_export.rb - there is class
defined without inheriting from ApplicationRecord
so when class from import_export.rb is loaded first
is it loaded as RssFeed < Object and when
after this is app/model/rss_feed.rb loaded
it is causing error(this warning is suppressed by
silence_warnings):

TypeError: superclass mismatch for class RssFeed

and then RssFeed < ApplicationRecord is not loaded

Sorting Dir.glob guarantee order so that
files will be first then subdirectories,..
() because '.'.ord < '/'.ord
(sort is also used in eager_load!
https://github.com/rails/rails/blob/master/railties/lib/rails/engine.rb#L475)
@miq-bot
Copy link
Member

miq-bot commented Mar 2, 2017

Checked commit lpichler@7b3d7b6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

Copy link
Member

@isimluk isimluk left a comment

Choose a reason for hiding this comment

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

👍 Thanks for digging down for the reason behind the sporadic test failure!

@jrafanie
Copy link
Member

jrafanie commented Mar 2, 2017

@lpichler @isimluk Wow, nice find!

Am I correct in assuming we'll be renaming or namespacing the RssFeed from the import/export in a future PR? This PR will fix that issue but we can't have two RssFeed classes.

@isimluk
Copy link
Member

isimluk commented Mar 2, 2017

Am I correct in assuming we'll be renaming or namespacing the RssFeed from the import/export in a future PR? This PR will fix that issue but we can't have two RssFeed classes.

Not necessarily, no. We have this patter in multiple places and it is not at all harmful.

There are two modes when ruby parses class.

  • first creation
  • re-opening and adding stuff

Note that the following re-opening is fine:

class RssFeed < Application Record
 ...
end
class RssFeed
 ... something else
end

while the following will result in an exception:

class RssFeed
 ...
end
class RssFeed < ApplicationRecord
.. something else
end

So, ruby does the check for us. The problem with this code was that it silences_warnings.

@kbrock kbrock added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 2, 2017
@kbrock kbrock merged commit bd8aa09 into ManageIQ:master Mar 2, 2017
@lpichler lpichler deleted the sort_files_to_require_for_coverage_reports branch March 3, 2017 06:54
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Mar 3, 2017
Related to ManageIQ#14128

We should either use RssFeed.class_eval to reopen the class to force
rails to load the RssFeed model first or the technique in this commit,
which is to just include the module in RssFeed.  Note, this module was
already being included so there was no reason to reopen the class also.

The existing layout of re-opening the RssFeed with different class
definitions:

rss_feed/import_export.rb:
class RssFeed

rss_feed.rb:
class RssFeed < ApplicationRecord

can cause `TypeError: superclass mismatch for class RssFeed` if
the import_export.rb is loaded first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants