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

5844 embargo specs #6114

Merged
merged 23 commits into from
Aug 7, 2023
Merged

5844 embargo specs #6114

merged 23 commits into from
Aug 7, 2023

Conversation

alishaevn
Copy link
Contributor

related

Summary

get the spec pipeline working again

Guidance for testing, such as acceptance criteria or new user interface behaviors:

Type of change (for release notes)

Add an appropriate notes-* label to the PR (or indicate here) that classifies this change.

Choose from:

  • notes-major Major Changes (Potentially breaking changes)
  • notes-minor New Features that are backward compatible
  • notes-deprecation Deprecations
  • notes-bugfix Bug Fixes
  • notes-valkyrie Valkyrie Progress
  • notes-docs Documentation
  • notes-container Containerization related (Docker, Helm, etc)

Detailed Description

More detailed description, if necessary. Try to be as descriptive as you can: even if you think that the PR content is obvious, it may not be obvious to others. Include tracebacks if helpful, and be sure to call out any bits of the PR that may be work-in-progress.

Description can have multiple paragraphs and you can use code examples inside:

class PostsController
  def index
    respond_with Post.limit(10)
  end
end

Changes proposed in this pull request:

@samvera/hyrax-code-reviewers

expect(page).to have_content work.title.first
expect(page).to have_selector '.alert-success', text: 'Collection was successfully updated.'
end
click_button "Add to collection" # opens the modal
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this spec is currently failing, but I do not believe it was related to my embargo work (#6098). I haven't been able to narrow down where the error was introduced though to try and be able and figure out, "easier", what broke it.

ref: commit where I commented these out with hesitation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now skipping the spec conditionally, with a reason.

dassie Screenshot 2023-08-07 at 9 43 11 AM
koppie Screenshot 2023-08-07 at 9 43 32 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref: #6135

expect(response).to have_http_status(:ok)
expect(response.content_type).to eq 'image/jpeg'
end
get Riiif::Engine.routes.url_helpers.image_path(file.id, size: size, format: 'jpg', channels: nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this spec is currently failing, but I do not believe it was related to my embargo work (#6098). I haven't been able to narrow down where the error was introduced though to try and be able and figure out, "easier", what broke it.

ref: commit where I commented these out with hesitation

Copy link
Member

Choose a reason for hiding this comment

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

This failure is for sure not related to embargo. It should have never been passing because Riiif looks up the file metadata but no file metadata is ever created. Before our valk work, riiif wasn’t working at all. So I don’t know how this could have ever passed in Valkyrie land.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now skipping the spec conditionally, with a reason.

dassie image
koppie image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref: #6135

…rb:72 because the failures are unrelated to the #5844 embargo work.

both of these specs were failing on main before #6098 was accidentally merged to main with a failing pipeline. in an attempt to get main back to a green pipeline I skipped all failing specs, including these two. ref: 58de8c6.

this pr was to fix the broken embargo specs from #6098. these two are still unrelated however and will remain skipped. only in valkyrie, as that is where they are failing. there is work being done on the valkyrie spec suite to fix its several problems. hopefully those fixes will also resolve these issues.

as for the riif spec specifically, @orangewolf did some further digging and found: "It should have never been passing because Riiif looks up the file metadata but no file metadata is ever created. Before our valk work, riiif wasn’t working at all. So I do not know how this could have ever passed in Valkyrie land."
@alishaevn alishaevn marked this pull request as ready for review August 7, 2023 15:26
Copy link
Member

@orangewolf orangewolf left a comment

Choose a reason for hiding this comment

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

Looking so good!

@alishaevn alishaevn merged commit 08ef6c9 into main Aug 7, 2023
@alishaevn alishaevn deleted the 5844-embargo-specs branch August 7, 2023 16:03
@alishaevn alishaevn linked an issue Aug 7, 2023 that may be closed by this pull request
@dlpierce dlpierce added the notes-valkyrie Release Notes: Valkyrie specific label Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-valkyrie Release Notes: Valkyrie specific valk-rc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot remove embargo from work
3 participants