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

(#1443) Remove redundant tests for SSL #1588

Merged
merged 4 commits into from May 1, 2021
Merged

Conversation

andreoss
Copy link
Contributor

Per #1443

  • Remove TrustManager & SSLContext initiazliation from test

@victornoel
Copy link
Collaborator

@andreoss as mentioned in the ticket, the job included replacing calls to external urls with using takes, see #1443 (comment)

@andreoss
Copy link
Contributor Author

@victornoel Moved it to Takes.

new FtRemote(new TkFork(new FkRegex("/", new TkText(data.asString())))).exec(
uri -> new Assertion<>(
"Must read bytes from HTTPS URL",
new TextOf(new InputOf(uri)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@andreoss this test seems to exist to test that it works with https, but I don't think that FtRemote is serving https at all…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@victornoel Than I suggest removing this test completely than. The code under test doesn't depend on protocol at all

Copy link
Collaborator

Choose a reason for hiding this comment

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

@andreoss ok, that makes sense, and since the code is relying on Java's std lib for http requests, there is no big reason for testing it. Thanks for looking into it

@andreoss andreoss changed the title (#1443) Remove unnecessary SSL initialization (#1443) Remove redundant tests for SSL May 1, 2021
@victornoel
Copy link
Collaborator

@rultor merge

@rultor
Copy link
Collaborator

rultor commented May 1, 2021

@rultor merge

@victornoel OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 4725444 into yegor256:master May 1, 2021
@rultor
Copy link
Collaborator

rultor commented May 1, 2021

@rultor merge

@victornoel Done! FYI, the full log is here (took me 9min)

@0crat 0crat added qa and removed 0crat/scope labels May 1, 2021
@0crat
Copy link
Collaborator

0crat commented May 1, 2021

@sereshqua/z please review this job completed by @marceloamadeu/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

@sereshqua
Copy link

@0crat quality bad

@0crat
Copy link
Collaborator

0crat commented May 5, 2021

Quality is low, no payment, see §31

@0crat 0crat added quality/bad and removed qa labels May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants