RedJocker / OrdersMenu

0 stars 0 forks source link

include assertions to prevent solutions with overlapping composables #11

Closed RedJocker closed 1 year ago

RedJocker commented 1 year ago

All stages of the project (except the first one) should be supplemented with an assertion that all composables do not overlap each other.

on revision by @Razotron

RedJocker commented 1 year ago

I'm a bit concerned with how costly this kind of test can be, since it will be testing every node against every node and doing some range iteration for each of these. I had some print statements set while writing isOnSameRowAs and it is already way more demanding than it looks, and it does look very very simple. But I guess it is worth at least seeing how such a test performs, we don't have to be much strict with test performance, but being too loose is also not good since users do have some expectations on how long tests take to run. I know you @Razotron have an already made implementation of a method that does this test, maybe you could send me a pull-request dealing with this issue?

RazoTRON commented 1 year ago

The average time to run a test without an overlap assertion on my laptop is exactly the same as with an overlap assertion (adjusted for margin of error). Would you like to see how it works in my project? Task №2: https://github.com/RazoTRON/PhotoGallery/blob/master/PhotoGallery/task2/src/test/java/org/hyperskill/photogallery/Test.kt

RedJocker commented 1 year ago

let me show you why I'm a bit concerned with how expensive this can be.

this is a tiny modification to isOnSameRowAs to count the number of times it is invoked.

var isOnSameRowAsCounter = 0

    fun isOnSameRowAs(otherNode: SemanticsNodeInteraction): SemanticsMatcher {
        return SemanticsMatcher(
            "is on same row as ${otherNode.printToString()
                .substringAfter("\n")
                .replace('\n', ' ')}"
        ) { node ->

            println("isOnSameRowAs $isOnSameRowAsCounter")
            isOnSameRowAsCounter++
            val otherNodeYPosition = otherNode.fetchSemanticsNode().positionInWindow.y
            val nodeYPosition = node.positionInWindow.y
            abs(otherNodeYPosition - nodeYPosition) < 10f
        }
    }

running stage5 tests:

test00 -> 0 test01 -> 74 test02 -> 0 test03 -> 324 test04 -> 2749 test05 -> 3349 test06 -> 2149

RedJocker commented 1 year ago

I had some problems using your implementation

https://github.com/RazoTRON/PhotoGallery/blob/0246afaed20f8883ff46dd76c5dc91fa2d3142d6/PhotoGallery/task2/src/test/java/org/hyperskill/photogallery/internals/Assertions.kt#L136-L167

It was failing a case in which the views were vertically aligned but in different rows and I also think it can fail if one view is contained in another depending on which view is inside and which is outside.

I also had problem with one same view being tested against itself.

I was able to find some ready made functions instead for checking the intersection, and also changed the sublist calculation

fun assertNotOverlapEachOthers(
        listOfNodes: List<SemanticsNode>
    ) {
        listOfNodes.forEachIndexed { i, node ->
            val subList = listOfNodes.drop(i + 1)
            subList.forEach { anotherNode ->

            val hasIntersection = anotherNode.boundsInWindow.intersect(node.boundsInWindow).let {
                // from intersect docstring
                // 'If the two rectangles do not overlap,
                // then the resulting Rect will have a negative width or height'
                it.width > 0 && it.height > 0
            }

            assert(!hasIntersection) {
                val nodeText = node.config.getOrNull(SemanticsProperties.Text).toString()
                val anotherNodeText = anotherNode.config.getOrNull(SemanticsProperties.Text).toString()
                "View with text $nodeText and bounds ${node.boundsInWindow}" +
                    " should not overlap " +
                    "view with text $anotherNodeText and bounds ${anotherNode.boundsInWindow}"
                }
            }
        }
    }

Another difference is that it is using List<SemanticsNode> as argument instead of List<SemanticsNodeInteraction>. At first I was only doing this because it was more convenient for me, but I'm thinking that pre-fetching nodes may be helping in reduce the cost. I am noticing that fetchSemanticsNode can be quite costly depending on the semanticMatchers complexity and the docstring from fetchSemanticsNode says

"Note: Accessing this object involves synchronization with your UI. If you are accessing this multiple times in one atomic operation, it is better to cache the result instead of calling this API multiple times."

for what I've been testing the performance for this assertion is ok

RazoTRON commented 1 year ago

This is a very elegant implementation!