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.

Sign up for GitHub

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

Jump to bottom

Add tests for broken link actions #3096

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Add tests for broken link actions #3096

merged 1 commit into from
Jan 17, 2025
+415 −1

Conversation

MizukiTemma
Copy link
Member

@MizukiTemma MizukiTemma commented Oct 2, 2024 •
edited
Loading

Short description

This PR adds tests for actions users can take in the broken link list.

Proposed changes

  • Do each action and check whether the target URLs/links are changed as expected.
  • Check whether actions taken in the netowork management are affecting all the targets system-wide, while actions within a region changing on the objects that belong to the region.

Side effects

  • Two assersions are disabled, as they are failling somehow (new url/link objects are not created after action). So far as I observed, both search&replace and URL replace are working fine. Probably a special care is needed for the link check listener?

Resolved issues

Fixes: #3062

Pull Request Review Guidelines

@codeclimate CodeClimate
Copy link

codeclimate bot commented Oct 2, 2024

Code Climate has analyzed commit a306330 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 82.9% (0.4% change).

View more on Code Climate.

@MizukiTemma MizukiTemma mentioned this pull request Nov 7, 2024
Merged
@MizukiTemma MizukiTemma mentioned this pull request Nov 14, 2024
Merged
david-venhoff
david-venhoff reviewed Nov 20, 2024
View reviewed changes
Copy link
Member

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

Thanks, this looks mostly good to me :)

Could you also add a test case for #3058?

integreat_cms/cms/views/linkcheck/linkcheck_list_view.py Outdated Show resolved Hide resolved
tests/cms/views/link_replace/test_link_actions.py Outdated Show resolved Hide resolved
tests/cms/views/link_replace/test_link_actions.py Show resolved Hide resolved
tests/cms/views/link_replace/test_link_actions.py Outdated Show resolved Hide resolved
@MizukiTemma MizukiTemma added not-testable Issues that are not testable and removed not-testable Issues that are not testable labels Dec 11, 2024
JoeyStk
JoeyStk approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@JoeyStk JoeyStk left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM! :)

@JoeyStk JoeyStk self-assigned this Jan 16, 2025
charludo
charludo approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

Thank you, looks great! 😄 (Also thanks for doing so many of these testing PRs in general, we are seriously in your debt for that :D)

tests/cms/views/link_replace/test_link_actions.py Outdated Show resolved Hide resolved
@MizukiTemma
Copy link
Member Author

@JoeyStk @charludo
Sorry, I should have left some explanation about the status of this PR: assertions checking new links exists after replacement are failing somehow and @PeterNerlich #s suggestion to use linkcheck.listeners leads to never ending test 🙈 See this conversation for more details.

@charludo
Copy link
Contributor

@JoeyStk @charludo Sorry, I should have left some explanation about the status of this PR: assertions checking new links exists after replacement are failing somehow and @PeterNerlich #s suggestion to use linkcheck.listeners leads to never ending test 🙈 See this conversation for more details.

Ah, OK. Hmm. In that case, I will just leave my approval as-is, because I have zero ideas 😂

@MizukiTemma MizukiTemma force-pushed the test/link_actions branch 3 times, most recently from df19efc to 6624f1d Compare January 17, 2025 14:58
@MizukiTemma @david-venhoff
Add test for broken link actions
766880a 
Co-authored-by: David Venhoff <david.venhoff@tuerantuer.org>
@MizukiTemma MizukiTemma mentioned this pull request Jan 17, 2025
Open
@MizukiTemma
Copy link
Member Author

@charludo @JoeyStk @PeterNerlich
Thank you for your help and idea for this PR. As it seems none of us can fix the problem quickly, I'll merge this PR but with the failing assertions disabled instead of skipping a whole test so that other aspects can be tested.

I opened an issue that documents this problem so we can come back one day and remove the disables. #3343

@MizukiTemma MizukiTemma merged commit 664ac8f into develop Jan 17, 2025
5 checks passed
@MizukiTemma MizukiTemma deleted the test/link_actions branch January 17, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoeyStk JoeyStk JoeyStk approved these changes

@charludo charludo charludo approved these changes

@david-venhoff david-venhoff Awaiting requested review from david-venhoff

@PeterNerlich PeterNerlich Awaiting requested review from PeterNerlich

@jonbulz jonbulz Awaiting requested review from jonbulz

@jarlhengstmengel jarlhengstmengel Awaiting requested review from jarlhengstmengel

Assignees

@JoeyStk JoeyStk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add test for actions in the broken link list
4 participants
@MizukiTemma @charludo @JoeyStk @david-venhoff