From 05f97cfadce1cf3174e3f3a07f7ef702647503d7 Mon Sep 17 00:00:00 2001 From: Stilgar Date: Fri, 21 May 2021 00:46:46 +0200 Subject: [PATCH] Fix Flaky Tests stop() is called too soon in some cases, download might not have been started and calling stop will leave the items in pending so only call stop if items have been completed or on error + improve shouldStop as AtomicBoolean + trace logging in case issue happens again --- .../rarchives/ripme/ripper/AbstractHTMLRipper.java | 10 +++++----- .../rarchives/ripme/ripper/AbstractJSONRipper.java | 6 +++--- .../com/rarchives/ripme/ripper/AbstractRipper.java | 11 +++++++---- .../java/com/rarchives/ripme/ripper/AlbumRipper.java | 6 +++--- .../ripme/tst/ripper/rippers/RippersTest.java | 7 +++++++ 5 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/rarchives/ripme/ripper/AbstractHTMLRipper.java b/src/main/java/com/rarchives/ripme/ripper/AbstractHTMLRipper.java index 3e3fdb18..4431f2cf 100644 --- a/src/main/java/com/rarchives/ripme/ripper/AbstractHTMLRipper.java +++ b/src/main/java/com/rarchives/ripme/ripper/AbstractHTMLRipper.java @@ -128,7 +128,7 @@ public abstract class AbstractHTMLRipper extends AbstractRipper { index += 1; LOGGER.debug("Found image url #" + index + ": " + imageURL); downloadURL(new URL(imageURL), index); - if (isStopped()) { + if (isStopped() || isThisATest()) { break; } } @@ -139,7 +139,7 @@ public abstract class AbstractHTMLRipper extends AbstractRipper { if (!textURLs.isEmpty()) { LOGGER.debug("Found description link(s) from " + doc.location()); for (String textURL : textURLs) { - if (isStopped()) { + if (isStopped() || isThisATest()) { break; } textindex += 1; @@ -293,10 +293,10 @@ public abstract class AbstractHTMLRipper extends AbstractRipper { * Queues multiple URLs of single images to download from a single Album URL */ public boolean addURLToDownload(URL url, File saveAs, String referrer, Map cookies, Boolean getFileExtFromMIME) { - // Only download one file if this is a test. - if (super.isThisATest() && - (itemsPending.size() > 0 || itemsCompleted.size() > 0 || itemsErrored.size() > 0)) { + // Only download one file if this is a test. + if (super.isThisATest() && (itemsCompleted.size() > 0 || itemsErrored.size() > 0)) { stop(); + itemsPending.clear(); return false; } if (!allowDuplicates() diff --git a/src/main/java/com/rarchives/ripme/ripper/AbstractJSONRipper.java b/src/main/java/com/rarchives/ripme/ripper/AbstractJSONRipper.java index d7e93fcb..44596702 100644 --- a/src/main/java/com/rarchives/ripme/ripper/AbstractJSONRipper.java +++ b/src/main/java/com/rarchives/ripme/ripper/AbstractJSONRipper.java @@ -141,10 +141,10 @@ public abstract class AbstractJSONRipper extends AbstractRipper { * Queues multiple URLs of single images to download from a single Album URL */ public boolean addURLToDownload(URL url, File saveAs, String referrer, Map cookies, Boolean getFileExtFromMIME) { - // Only download one file if this is a test. - if (super.isThisATest() && - (itemsPending.size() > 0 || itemsCompleted.size() > 0 || itemsErrored.size() > 0)) { + // Only download one file if this is a test. + if (super.isThisATest() && (itemsCompleted.size() > 0 || itemsErrored.size() > 0)) { stop(); + itemsPending.clear(); return false; } if (!allowDuplicates() diff --git a/src/main/java/com/rarchives/ripme/ripper/AbstractRipper.java b/src/main/java/com/rarchives/ripme/ripper/AbstractRipper.java index 3653b9f0..465debe5 100644 --- a/src/main/java/com/rarchives/ripme/ripper/AbstractRipper.java +++ b/src/main/java/com/rarchives/ripme/ripper/AbstractRipper.java @@ -14,6 +14,8 @@ import java.util.List; import java.util.Map; import java.util.Observable; import java.util.Scanner; +import java.util.concurrent.atomic.AtomicBoolean; + import org.apache.log4j.FileAppender; import org.apache.log4j.Logger; import org.jsoup.HttpStatusException; @@ -47,17 +49,18 @@ public abstract class AbstractRipper public boolean hasASAPRipping() { return false; } // Everytime addUrlToDownload skips a already downloaded url this increases by 1 public int alreadyDownloadedUrls = 0; - private boolean shouldStop = false; + private final AtomicBoolean shouldStop = new AtomicBoolean(false); private static boolean thisIsATest = false; public void stop() { - shouldStop = true; + LOGGER.trace("stop()"); + shouldStop.set(true); } public boolean isStopped() { - return shouldStop; + return shouldStop.get(); } protected void stopCheck() throws IOException { - if (shouldStop) { + if (shouldStop.get()) { throw new IOException("Ripping interrupted"); } } diff --git a/src/main/java/com/rarchives/ripme/ripper/AlbumRipper.java b/src/main/java/com/rarchives/ripme/ripper/AlbumRipper.java index f433e77f..f245ba62 100644 --- a/src/main/java/com/rarchives/ripme/ripper/AlbumRipper.java +++ b/src/main/java/com/rarchives/ripme/ripper/AlbumRipper.java @@ -51,10 +51,10 @@ public abstract class AlbumRipper extends AbstractRipper { * Queues multiple URLs of single images to download from a single Album URL */ public boolean addURLToDownload(URL url, File saveAs, String referrer, Map cookies, Boolean getFileExtFromMIME) { - // Only download one file if this is a test. - if (super.isThisATest() && - (itemsPending.size() > 0 || itemsCompleted.size() > 0 || itemsErrored.size() > 0)) { + // Only download one file if this is a test. + if (super.isThisATest() && (itemsCompleted.size() > 0 || itemsErrored.size() > 0)) { stop(); + itemsPending.clear(); return false; } if (!allowDuplicates() diff --git a/src/test/java/com/rarchives/ripme/tst/ripper/rippers/RippersTest.java b/src/test/java/com/rarchives/ripme/tst/ripper/rippers/RippersTest.java index c09b8018..9e77a605 100644 --- a/src/test/java/com/rarchives/ripme/tst/ripper/rippers/RippersTest.java +++ b/src/test/java/com/rarchives/ripme/tst/ripper/rippers/RippersTest.java @@ -32,6 +32,13 @@ public class RippersTest { ripper.setup(); ripper.markAsTest(); ripper.rip(); + if (logger.isTraceEnabled()) { + logger.trace("working dir: " + ripper.getWorkingDir()); + logger.trace("list files: " + ripper.getWorkingDir().listFiles().length); + for (int i = 0; i < ripper.getWorkingDir().listFiles().length; i++) { + logger.trace(" " + ripper.getWorkingDir().listFiles()[i]); + } + } Assertions.assertTrue(ripper.getWorkingDir().listFiles().length >= 1, "Failed to download a single file from " + ripper.getURL()); } catch (IOException e) {