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/moab: remove cruft (inspired by rubocop) #32

Merged
merged 11 commits into from
Sep 7, 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

@ndushay ndushay force-pushed the clean-moab-specs1 branch 4 times, most recently from 77c5ddd to f27e0df Compare September 1, 2017 09:59
@sul-dlss sul-dlss deleted a comment from coveralls Sep 1, 2017
@sul-dlss sul-dlss deleted a comment from coveralls Sep 1, 2017
@sul-dlss sul-dlss deleted a comment from coveralls Sep 1, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 99.28% when pulling f27e0df on clean-moab-specs1 into beb7dfa on master.

@sul-dlss sul-dlss deleted a comment from coveralls Sep 5, 2017
@sul-dlss sul-dlss deleted a comment from coveralls Sep 5, 2017
@coveralls
Copy link

coveralls commented Sep 5, 2017

Coverage Status

Coverage decreased (-0.04%) to 99.276% when pulling 8822b9a on clean-moab-specs1 into 368c74a 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.

two or three small nitpicks, but looks great overall. definitely an improvement in readability and maintainability, AFAICT.

end

describe '=========================== INSTANCE ATTRIBUTES ===========================' do
context '#datetime' do
let(:version_metadata_event) { Moab::VersionMetadataEvent.new([]) }
Copy link
Member

Choose a reason for hiding this comment

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

probably doesn't make a practical difference since it's empty anyway, but seems like this should use Moab::VersionMetadataEvent.new({}), since the constructor wants a hash?

it 'populated options hash' do
opts = {
digital_object_id: 'Test digital_object_id',
versions: double(Moab::VersionMetadataEntry.name)
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 a list, like it was previously? e.g. versions: [double(Moab::VersionMetadataEntry.name)]

context '#versions' do
it 'empty Array if value not set' do
expect(version_metadata.versions).to be_kind_of Array
expect(version_metadata.versions.size).to eq 0
Copy link
Member

Choose a reason for hiding this comment

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

this seems duplicative of lines 13-17? if so, i'd be inclined to remove one or the other, though no preference on my part as to which one.

@jmartin-sul jmartin-sul deleted the clean-moab-specs1 branch September 7, 2017 00:14
@coveralls
Copy link

coveralls commented Sep 7, 2017

Coverage Status

Coverage decreased (-0.04%) to 99.275% when pulling 8f16289 on clean-moab-specs1 into 368c74a on master.

@coveralls
Copy link

coveralls commented Sep 7, 2017

Coverage Status

Coverage decreased (-0.04%) to 99.275% when pulling 8f16289 on clean-moab-specs1 into 368c74a on master.

@coveralls
Copy link

coveralls commented Sep 7, 2017

Coverage Status

Coverage decreased (-0.04%) to 99.276% when pulling a973abc on clean-moab-specs1 into 368c74a on master.

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