Skip to content

Improve Futures class code coverage #564

Merged
yegor256 merged 1 commit intocqfn:masterfrom
Ivruix:new-futures-test
Jan 17, 2025
Merged

Improve Futures class code coverage #564
yegor256 merged 1 commit intocqfn:masterfrom
Ivruix:new-futures-test

Conversation

@Ivruix
Copy link
Contributor

@Ivruix Ivruix commented Jan 17, 2025

Added test for asString method.

@Ivruix
Copy link
Contributor Author

Ivruix commented Jan 17, 2025

While doing so I've noticed some inconsistencies with the ordering of the group and artifact variables. In the Futures class itself there is this line:

Logger.info(
    this, "Started processing of %s:%s...",
    group, artifact
);

And later, with the artifact and group swapped:

Logger.info(
    this, "Finished processing of %s:%s",
    artifact, group
);

Also in the FuturesTest.java file in testSimpleScenario method the order of artifact and group is incorrect, as the Future class expects the first argument to be group:

new Futures(
    (artifact, group) -> input -> new RsPage(
        new RqFake(),
        "wait",
        () -> new IterableOf<>(
            new XeAppend("group", group),
            new XeAppend("artifact", artifact)
        )
    )
).apply("a", "g").get().apply("test"),

It doesn't matter for the purposes of the test, but it might confuse someone, as it did for me. I can make a small PR fixing these issues if you'd like.

@Ivruix
Copy link
Contributor Author

Ivruix commented Jan 17, 2025

@yegor256 please take a look. I'm not sure why ubuntu builds failed, but others have passed. They timed out on AsyncReportsTest test which I did not modify.

@yegor256 yegor256 merged commit 3b0a64d into cqfn:master Jan 17, 2025
10 of 12 checks passed
@yegor256
Copy link
Member

@Ivruix thanks for this one! Indeed, will be great if you fix the ordering in a new PR

@Ivruix Ivruix deleted the new-futures-test branch January 18, 2025 08:46
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