-
Notifications
You must be signed in to change notification settings - Fork 900
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
Refactor MiqAeExportSpec to decrease test run time #72
Refactor MiqAeExportSpec to decrease test run time #72
Conversation
@mkanoor @tinaafitz @gmcculloug Please review. |
Also in the process of getting rid of some of these rubocop warnings. Will likely not change most of the ones that are relevant to code I didn't modify but simply indented/moved. |
Leaving the rest of the Rubocops here. For miq_ae_instance I would rather use the { } for blocks since we're calling a method on the end of the block. For miq_ae_class_spec it's simply a long string of XML that I figured didn't need to be broken up into separate lines via the | operator. I will change it if that's a style thing we're going for, though. miq_ae_field_spec and miq_ae_instance_spec warnings/style is from simply indenting code that did not otherwise change. |
This pull request is not mergeable. Please rebase and repush. |
xml_attrs = { :name => self.name, :namespace => self.namespace } | ||
|
||
self.class.column_names.each { |cname| | ||
# Remove any columns that we do not want to export | ||
next if ["id", "created_on", "updated_on", "updated_by", "reserved"].include?(cname) || cname.ends_with?("_id") | ||
next if %w(id created_on updated_on updated_by).include?(cname) || cname.ends_with?("_id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was reserved
removed from the list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I wrote the test, I tried to set up the different models with a "reserved" attribute, and it came back saying that column does not exist. So, originally it must've been copy/pasted from something that did have a reserved attribute.
Since Travis is now green, please rebase to ensure that this PR continues to keep our tests passing. |
This pull request is not mergeable. Please rebase and repush. |
@@ -1,156 +1,196 @@ | |||
require "spec_helper" | |||
require "debugger" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be here. It fails the specs because it's not in the Gemfile (and shouldn't be)
This pull request is not mergeable. Please rebase and repush. |
Checked commit eclarizio@08658d6 with rubocop 0.21.0 vmdb/app/models/miq_ae_instance.rb vmdb/spec/models/miq_ae_class_spec.rb
vmdb/spec/models/miq_ae_datastore_spec.rb
vmdb/spec/models/miq_ae_field_spec.rb
vmdb/spec/models/miq_ae_instance_spec.rb
|
Refactor MiqAeExportSpec to decrease test run time
Update provision requirements check to allow exact matches (cherry picked from commit 48424c966ddbd2e58b536d4fa239a40466e8280f) https://bugzilla.redhat.com/show_bug.cgi?id=1481449
Update provision requirements check to allow exact matches (cherry picked from commit 48424c966ddbd2e58b536d4fa239a40466e8280f) https://bugzilla.redhat.com/show_bug.cgi?id=1481869
Update provision requirements check to allow exact matches (cherry picked from commit 48424c966ddbd2e58b536d4fa239a40466e8280f) https://bugzilla.redhat.com/show_bug.cgi?id=1481449
MiqAeExportSpec was our slowest spec, and these refactorings should turn the 30+ seconds that it used to take into about a second. Running all of the changed files (with some old tests that are still setting too much stuff up) takes about 4 seconds, so I'd consider this a significant speed up.
I also noticed that it was attempting to call to_export_xml on instances of CustomButton, but that method does not exist at all on CustomButton. It's implemented here as a placeholder, as I don't know enough about what should or should not be exported from a CustomButton.