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

spec/unit_tests/serializer: remove cruft (inspired by rubocop) #31

Merged
merged 1 commit into from
Sep 5, 2017

Conversation

ndushay
Copy link
Contributor

@ndushay ndushay commented Sep 1, 2017

Please review especially for:

  • no useful tests removed (some pointless ones have been removed)
  • no existing test functionality has been changed (refactored some variable names or avoided long lines or changed otherwise for readability)

This spec cleanup:

  • shortens names of specs to be just the method names
  • removes comments containing the rdoc or implementation of methods
  • removes specs that are useless (e.g. that setting an attribute value means you can retrieve the attribute value; that instantiating an object returns an instance of the object)
  • splits out multiple tests in a single spec into separate tests
  • avoids long lines
  • refactors variable names for clarity
  • removes unnecessary @ variables
  • tried to follow principles to make rubocop happier
    etc.

Connected to #6

@coveralls
Copy link

coveralls commented Sep 1, 2017

Coverage Status

Coverage increased (+0.002%) to 99.319% when pulling 613ab12 on clean-serializer-specs into beb7dfa on master.

Copy link
Member

@jmartin-sul jmartin-sul left a comment

Choose a reason for hiding this comment

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

one pretty minor quibble, but otherwise, looks good to me.

# def self.xml_pathname_exist?(parent_dir, filename=nil)
# self.xml_pathname(parent_dir, filename).exist?
# end
it 'both parent_dir and pathname specified' do
Copy link
Member

Choose a reason for hiding this comment

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

seems like this should be filename instead of pathname here and in the prior test name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch - thanks. fixing now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.313% when pulling 1a311a6 on clean-serializer-specs into aca484d on master.

@jmartin-sul jmartin-sul merged commit 368c74a into master Sep 5, 2017
@jmartin-sul jmartin-sul deleted the clean-serializer-specs branch September 5, 2017 18:41
@sul-dlss sul-dlss deleted a comment from coveralls Sep 5, 2017
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.

None yet

3 participants