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

Added LCMPT and AFSET sub-vocabularies to LOC #60

Merged
merged 1 commit into from
Jun 27, 2014
Merged

Added LCMPT and AFSET sub-vocabularies to LOC #60

merged 1 commit into from
Jun 27, 2014

Conversation

scande3
Copy link
Contributor

@scande3 scande3 commented Jun 27, 2014

Added LCMPT and AFSET sub-vocabularies to the
Library of Congress vocabulary. In addition,
fixed a slight name misspelling on the README.

@scande3
Copy link
Contributor Author

scande3 commented Jun 27, 2014

This is related to issue #57

@scande3
Copy link
Contributor Author

scande3 commented Jun 27, 2014

Yes, I tested these via both the console and web url.

I can add a canned spec test with Webmock but the general functionality is the same as the other mocked out vocabulary in the sparse tests. (Only two are currently mocked out and one of them passes despite currently not working with live data - see issue #61 ).

As the test will always pass being mocked out if either of the two testing the url formatting and standard LOC type of responses work, not sure what I'd be testing in this case. Could you elaborate? I can add tests that prove the vocabulary exists as an option with the correct URL? Or add a subset of live, non-mocked tests? Thanks!

@awead
Copy link
Contributor

awead commented Jun 27, 2014

Let's do a test for the existence of the vocab with the correct URL. We need just a little coverage so no one inadvertently breaks something later. I think you're point about #61 is a broader issue with testing in general, which I'm not sure how to solve. More thinking needed there....

@scande3
Copy link
Contributor Author

scande3 commented Jun 27, 2014

@awead : I've added unit tests that confirm the existence and urls of the various sub vocabularies. I've also re-organized the unit tests a bit (some were duplicates and all of the examples are located together now). I also updated one of the example response files since I couldn't get the exact response the original example had.

Let me know if this works or requires further fixing (or if this reorganization goes too far). Thanks.

@scande3
Copy link
Contributor Author

scande3 commented Jun 27, 2014

(Shows failed in three of four rails versions in travis... will update in a second to see if I can avoid the webmock race condition?)

@scande3
Copy link
Contributor Author

scande3 commented Jun 27, 2014

(Seems like it is fixed with all four rails builds passing)

Added LCMPT and AFSET sub-vocabularies to the
Library of Congress vocabulary. In addition,
fixed a slight name misspelling on the README.
@awead
Copy link
Contributor

awead commented Jun 27, 2014

👏 nice!

awead added a commit that referenced this pull request Jun 27, 2014
Added LCMPT and AFSET sub-vocabularies to LOC
@awead awead merged commit b49bc51 into samvera:master Jun 27, 2014
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