Removes not needed guard #103

valeriyvan · 2019-01-04T16:15:44Z

max func works consistently on a sequence of any length.

From documentation:

Returns
The sequence’s maximum element if the sequence is not empty; otherwise, nil.

ghost · 2019-01-04T16:23:09Z

	1 Warning
⚠️	The use of Swift 3 @objc inference in Swift 4 mode is deprecated. Please address deprecated @objc inference warnings, test your code with “Use of deprecated Swift 3 @objc inference” logging enabled, and then disable inference by changing the “Swift 3 @objc Inference” build setting to “Default” for the “FBSnapshotTestCase iOS” target. (in target ‘FBSnapshotTestCase iOS’)

	1 Message
📖	Executed 49 tests, with 0 failures (0 unexpected) in 19.752 (19.810) seconds

Current coverage for WeScan is `39.48%`

Files changed	-	-
Array+Utils.swift	`100.00%`	✅

Current coverage for WeScanSampleProject is `42.86%`

✅ No files affecting coverage found

Powered by xcov

Generated by 🚫 Danger

julianschiavo · 2019-01-05T01:01:12Z

Wouldn’t removing the guard make it return nil if there’s only one Quad instead of the current behavior of returning the only Quad?

valeriyvan · 2019-01-05T21:25:17Z

No. max and min functions on Sequence are consistent for any length.
If you are interested you could look at implementation, starts on line 155.

valeriyvan · 2019-01-05T21:29:23Z

@justjs, added test to make you sure.

julianschiavo · 2019-01-06T05:55:59Z

Thanks for the addition of the test, but what I meant by my previous comment is this:

On the old implementation, if you had 2 or more rectangles, it would have returned the bigger one. If you had 1 rectangle, it would return the only rectangle.

On the new implementation, as per the documentation (The sequence’s maximum element if the sequence is not empty; otherwise, nil.), if you have only 1 rectangle, it will now return nil rather than the only rectangle.

This is also demonstrated by the testCorrectlyDetectsAndReturnsQuadilateral test failing.

valeriyvan · 2019-01-06T11:23:05Z

julianschiavo · 2019-01-06T11:24:01Z

Yes, that test is passing because it is using 10 rectangles. The CI is failing on the testCorrectlyDetectsAndReturnsQuadilateral test, which is the existing/old test.

valeriyvan · 2019-01-06T11:25:17Z

Ops, I did miss this part!

valeriyvan · 2019-01-06T11:26:10Z

About max being consistent with any sequence length.

julianschiavo · 2019-01-06T11:27:17Z

That's weird, seems to be incorrect according to the documentation. However, if that's how it works this PR should work, not sure why the Travis is failing.

valeriyvan · 2019-01-06T11:32:42Z

No, documentation is correct!
It says max return nil only for empty sequence, otherwise it returns maximum element. Single element sequence is not empty. For single element sequence that single element is max element (and min element as well). That how max/min work.
But if documentation might me misunderstood that the reason improve it, I guess.

AvdLee

Thanks for the contribution! Can you verify why CI is failing?

valeriyvan · 2019-01-09T09:42:17Z

Sure

julianschiavo · 2019-01-09T10:00:27Z

I’ve reviewed the CI error and it seems there was some kind of issue with the snapshot tests. If you’ve try run them locally maybe you can see what the issue is clearer. (In your project folder just run “git submodules init; git submodules update” to install the library used for the tests)

…ture func on CIDetector fails to find anything on 50x image but succeeds on 150x. Failure lead to force unwrap of nil in tests.

…nc with CIRectangleDetectorTests.swift

…sionRectangleDetectorTests.swift

…in tests to avoid FBSnapshotTestCase mistakingly looking for reference images in ReferenceImages_32 instead of ReferenceImages_64

julianschiavo · 2019-01-09T10:41:44Z

Hey @valeriyvan, while I understand you are trying to fix the tests, agnosticOptions is only on the CIRectangleDetector tests because it was updated between iOS versions - so this has to stay there or the tests will fail.

Also, I think it would be better to create a new PR for this, especially as it seems these are improvements to the tests rather than fixes (as the tests pass on all other PRs).

Hope that helps 🙂

(Edit: Also, changing the size is not a good idea - while it may fix the tests we should aim to fix the underlying code instead!)

valeriyvan · 2019-01-09T11:01:46Z

All tests passing locally. On the server one of snapshot tests fails with failure of loading reference image. FBSnapshotTestCase looks for reference image in ReferenceImages_32 when it is in ReferenceImages_64. According to documentation it should look both before failing.
Anyway, failing tests have nothing in common with the reason of this PR.

valeriyvan · 2019-01-09T11:04:26Z

Guys, @AvdLee requests fixing tests, @justjs requests fixing tests in separate PR. Can't satisfy both of you.

valeriyvan · 2019-01-09T11:06:21Z

Anyway, I can't fix failing test at the moment. Snapshot test still looks for reference image in wrong directory. And tests are passing locally.

julianschiavo · 2019-01-09T11:25:23Z

You should listen to @AvdLee over me - I don't work for WeTransfer. But I understood it as fixing the PR, which was causing the tests to fail, while it seems like here the tests have been changed to make them pass.

valeriyvan · 2019-01-09T11:33:04Z

The reason of tests failing is not related to this PR.

julianschiavo · 2019-01-09T11:34:19Z

I don't want to be hostile/create an argument here, but recently merged PRs did not have this issue, so it does seem to be an issue with this PR: #104 and #102 (both made by you) did not have this issue.

valeriyvan · 2019-01-09T11:44:20Z

I don't see this PR as breaking change. It was innocent change on its own.
"After" doesn't mean "because of".
My be I am wrong and missing something.
One of reason of tests failed was crash because of force unwrap of nil in test itself. This was fixed by fixing test itself. It wasn't related to PR.
Second reason - snapshot test case looking wrong dir for reference image. Not related to PR as well. It was fixed by fixing tests as well.
All tests are passing locally. I have no idea why snapshot test case is still looking wrong dir at server.

AvdLee · 2019-01-09T16:10:49Z

Just to make sure, did you merge in the latest changes from develop into your branch? If so, it's our responsibility to fix CI and make it work again.

valeriyvan · 2019-01-09T16:28:24Z

Merged up to date develop

valeriyvan · 2019-01-09T18:43:28Z

Can't believe my eyes.

julianschiavo

As you were able to resolve the issues by merging develop, could we remove all the changes to the test in this PR to keep it focused? If the issue was merging develop it should all work, but you can try to remove parts of your changes slowly to see if everything still works. 🙂

valeriyvan · 2019-01-11T13:12:58Z

Merge of develop haven't resolved issue with broken tests.
Accidentally switching Simulator from iPhone X to iPhone 7 and running tests I have found that tests started to fail locally. Update of reference images fixed broken tests. Now tests pass on only locally bon on CI as well.

julianschiavo · 2019-01-11T13:14:08Z

Yes, because you changed the settings for the tests so the old reference images were outdated.

valeriyvan · 2019-02-14T12:55:37Z

Any reason why this isn't being merged?

Boris-Em · 2019-02-14T14:56:35Z

Waiting on @justjs's approval.

julianschiavo

@Boris-Em I see no issue with this - only thing is that we should be careful as we're changing the reference images.

AvdLee · 2019-02-15T08:33:23Z

@justjs as long as our tests are succeeding, this shouldn't be a problem. Do you agree?

julianschiavo · 2019-02-15T09:00:03Z

Fair enough, I’m ok with that. Let’s make sure to keep this in mind in case was have future issues with the tests 👍

Removes not needed guard

511ee9f

Adds test for biggest for single element array

f59f3cc

Changes count of rectangles to 1 in test

937735b

AvdLee requested changes Jan 8, 2019

View reviewed changes

valeriyvan added 6 commits January 9, 2019 11:37

Increases targetSize from 50x to 150x because for whatever reason fea…

42dbbfd

…ture func on CIDetector fails to find anything on 50x image but succeeds on 150x. Failure lead to force unwrap of nil in tests.

Changes Increases targetSize from 50x to 150x to make this test in sy…

524b88b

…nc with CIRectangleDetectorTests.swift

Removes non need Void variable to stop Xcode complaining

c9b6311

Removes agnosticOptions of snapshot test to make test in sync with Vi…

7b3606f

…sionRectangleDetectorTests.swift

Updates sample images for snapshot tests

e333af1

Sets scale to 0.0 in calls of UIGraphicsBeginImageContextWithOptions …

c1fef29

…in tests to avoid FBSnapshotTestCase mistakingly looking for reference images in ReferenceImages_32 instead of ReferenceImages_64

Reverts agnosticOptions and updates related reference image

1be5f07

Merge branch 'develop' into biggest

9883a67

Updating reference images for snapshot tests

5e2017a

julianschiavo suggested changes Jan 10, 2019

View reviewed changes

AvdLee approved these changes Jan 14, 2019

View reviewed changes

Merge branch 'develop' into biggest

cd1f961

Boris-Em approved these changes Feb 14, 2019

View reviewed changes

julianschiavo approved these changes Feb 14, 2019

View reviewed changes

Merge branch 'develop' into biggest

7074395

AvdLee merged commit a26664e into WeTransfer:develop Feb 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes not needed guard #103

Removes not needed guard #103

valeriyvan commented Jan 4, 2019

ghost commented Jan 4, 2019 •

edited by ghost

julianschiavo commented Jan 5, 2019

valeriyvan commented Jan 5, 2019 •

edited

valeriyvan commented Jan 5, 2019

julianschiavo commented Jan 6, 2019

valeriyvan commented Jan 6, 2019

julianschiavo commented Jan 6, 2019

valeriyvan commented Jan 6, 2019

valeriyvan commented Jan 6, 2019

julianschiavo commented Jan 6, 2019

valeriyvan commented Jan 6, 2019

AvdLee left a comment

valeriyvan commented Jan 9, 2019

julianschiavo commented Jan 9, 2019

julianschiavo commented Jan 9, 2019 •

edited

valeriyvan commented Jan 9, 2019

valeriyvan commented Jan 9, 2019 •

edited

valeriyvan commented Jan 9, 2019 •

edited

julianschiavo commented Jan 9, 2019

valeriyvan commented Jan 9, 2019

julianschiavo commented Jan 9, 2019

valeriyvan commented Jan 9, 2019 •

edited

AvdLee commented Jan 9, 2019

valeriyvan commented Jan 9, 2019

valeriyvan commented Jan 9, 2019

julianschiavo left a comment

valeriyvan commented Jan 11, 2019

julianschiavo commented Jan 11, 2019

valeriyvan commented Feb 14, 2019

Boris-Em commented Feb 14, 2019

julianschiavo left a comment

AvdLee commented Feb 15, 2019

julianschiavo commented Feb 15, 2019

Removes not needed guard #103

Removes not needed guard #103

Conversation

valeriyvan commented Jan 4, 2019

ghost commented Jan 4, 2019 • edited by ghost

Current coverage for WeScan is 39.48%

Current coverage for WeScanSampleProject is 42.86%

julianschiavo commented Jan 5, 2019

valeriyvan commented Jan 5, 2019 • edited

valeriyvan commented Jan 5, 2019

julianschiavo commented Jan 6, 2019

valeriyvan commented Jan 6, 2019

julianschiavo commented Jan 6, 2019

valeriyvan commented Jan 6, 2019

valeriyvan commented Jan 6, 2019

julianschiavo commented Jan 6, 2019

valeriyvan commented Jan 6, 2019

AvdLee left a comment

Choose a reason for hiding this comment

valeriyvan commented Jan 9, 2019

julianschiavo commented Jan 9, 2019

julianschiavo commented Jan 9, 2019 • edited

valeriyvan commented Jan 9, 2019

valeriyvan commented Jan 9, 2019 • edited

valeriyvan commented Jan 9, 2019 • edited

julianschiavo commented Jan 9, 2019

valeriyvan commented Jan 9, 2019

julianschiavo commented Jan 9, 2019

valeriyvan commented Jan 9, 2019 • edited

AvdLee commented Jan 9, 2019

valeriyvan commented Jan 9, 2019

valeriyvan commented Jan 9, 2019

julianschiavo left a comment

Choose a reason for hiding this comment

valeriyvan commented Jan 11, 2019

julianschiavo commented Jan 11, 2019

valeriyvan commented Feb 14, 2019

Boris-Em commented Feb 14, 2019

julianschiavo left a comment

Choose a reason for hiding this comment

AvdLee commented Feb 15, 2019

julianschiavo commented Feb 15, 2019

ghost commented Jan 4, 2019 •

edited by ghost

Current coverage for WeScan is `39.48%`

Current coverage for WeScanSampleProject is `42.86%`

valeriyvan commented Jan 5, 2019 •

edited

julianschiavo commented Jan 9, 2019 •

edited

valeriyvan commented Jan 9, 2019 •

edited

valeriyvan commented Jan 9, 2019 •

edited

valeriyvan commented Jan 9, 2019 •

edited