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

Fast Grid Dump #1876

Open
mvysny opened this issue Mar 7, 2025 · 6 comments
Open

Fast Grid Dump #1876

mvysny opened this issue Mar 7, 2025 · 6 comments

Comments

@mvysny
Copy link
Member

mvysny commented Mar 7, 2025

Feature request: I need to test whether the Grid is showing correct data, and for that I need to capture the contents which the Grid is showing, and compare the result against the expected Grid contents, which is just a two-dimensional array of Strings.

Currently what I can do is to call GridElement.getRows(), then grab all cells for each row, then call GridTHTDElement.getText(). However this generates one browser request per every cell, which is hopelessly slow.

It would be great if GridElement offered a faster way to dump the cells into a 2-dimensional array of Strings: perhaps by dumping/calling the DataProvider directly, or by capturing currently shown data from its HTML structure - can be done page-by-page way.

We need this for Vaadin 23/TestBench 23 please.

@TatuLund
Copy link
Contributor

TatuLund commented Mar 7, 2025

It would be great if GridElement offered a faster way to dump the cells into a 2-dimensional array of Strings: perhaps by dumping/calling the DataProvider directly,

I think this is out of scope of TestBench browser tests and should be done with TestBench UI Unit Test instead.

@mvysny
Copy link
Member Author

mvysny commented Mar 7, 2025

I think this is out of scope of TestBench browser tests and should be done with TestBench UI Unit Test instead.

Perhaps... But then again, the point of Grid is to show the data, and the point of TestBench is to assert on the presence and state of components. In such a context, what sense has GridElement if one can't reasonably assert on the contents of the grid element?

@TatuLund
Copy link
Contributor

TatuLund commented Mar 7, 2025

It is not a question is it possible or not. With current API it is possible already. It is just about performance regarding special cases.

Here is one approach of producing two dimensional collection of the WebElements representing slotted content in the Grid. I compared the ten run average run time of the test code only excluding the test start and shutdown time.

    @Test
    public void assessGridContentTest0() {

        GridElement grid = $(GridElement.class).first();

        var rows = grid.getVisibleRows().size();
        var columns = grid.getVisibleColumns().size();

        repeat10(() -> {
            List<List<String>> elems = (List<List<String>>) grid
                    .getCommandExecutor()
                    .executeScript("const grid = arguments[0];"
                            + "return grid._getVisibleRows().map(row => Array.from(row.children).map(cell => grid.querySelector('[slot='+cell.children[0].name+']').innerText));",
                            grid);

            for (int i = 0; i < rows; i++) {
                for (int j = 0; j < columns; j++) {
                    assertNotNull(elems.get(i).get(j));
                }
            }
        });
    }

    @Test
    public void assessGridContentTest1() {

        GridElement grid = $(GridElement.class).first();

        var rows = grid.getVisibleRows().size();
        var columns = grid.getVisibleColumns().size();

        repeat10(() -> {
            List<List<WebElement>> elems = (List<List<WebElement>>) grid
                    .getCommandExecutor()
                    .executeScript("const grid = arguments[0];"
                            + "return grid._getVisibleRows().map(row => Array.from(row.children).map(cell => grid.querySelector('[slot='+cell.children[0].name+']')));",
                            grid);

            for (int i = 0; i < rows; i++) {
                for (int j = 0; j < columns; j++) {
                    assertNotNull(elems.get(i).get(j).getText());

                }
            }
        });
    }

    @Test
    public void assessGridContentTest2() {

        GridElement grid = $(GridElement.class).first();

        var rows = grid.getVisibleRows().size();
        var columns = grid.getVisibleColumns().size();

        repeat10(() -> {
            for (int i = 0; i < rows; i++) {
                for (int j = 0; j < columns; j++) {
                    assertNotNull(grid.getCell(i, j).getText());
                }
            }
        });
    }

    private void repeat10(Runnable runnable) {
        var totalTime = 0;
        for (int count = 0; count < 10; count++) {
            var time = System.currentTimeMillis();
            runnable.run();
            var time2 = System.currentTimeMillis();
            var timeDiff = time2 - time;
            totalTime += timeDiff;
        }
        System.out.println("Average time: " + (totalTime / 10));

}

The results were

assessGridContentTest0: 0.1 sec
assessGridContentTest1: 4.5 sec
assessGridContentTest2: 15.2 sec

So it looks like the execution time if the test is 1/3 of the current method.

@mvysny
Copy link
Member Author

mvysny commented Mar 11, 2025

It is just about performance regarding special cases.

I don't think that asserting against the state of the grid is a special case: this is actually the most common case you use TestBench for. And, if the performance of that can be improved by a factor of 50, that is quite a significant improvement which turns "unusable/too slow" into "usable".

@mvysny
Copy link
Member Author

mvysny commented Mar 28, 2025

It is not a question is it possible or not. With current API it is possible already. It is just about performance regarding special cases.

This is basically the same thing as defending a program which runs n+1 SQL selects instead of one SQL with a join. Same argument can be made: "with current API it is possible already. It is just about performance regarding special cases.", leading to absurd result.

TestBench API is broken by design - it forces you to run n+1 queries for anything more complex than the most basic stuff.

@mvysny
Copy link
Member Author

mvysny commented Mar 28, 2025

As it turns out, we'll need the HTML contents of the Grid cell, so the grid dump functionality should return both.

The best solution would be an Iterable<GridRow> which would lazily fetch pages of cells as the user iterates through that. Advantages:

  1. You can stop the iteration at any time: if the table is huge, you have a choice to assert against first 10 or 100 rows.
  2. The iterator would fetch an entire page of cells in one browser request, making it very fast.
  3. The GridRow could be a list of GridCell which would simply offer innerHTML of the cell; a function getText() could use Jsoup to parse the HTML and retrieve the text.

A prototype of such iterator follows. Note that this iterator has a bad performance of making O(nm) requests to the browser, so it should serve only as a case study.

public class PagedGridTRElementIterator implements Iterator<GridTRElement> {
    @NotNull
    private final GridElement grid;
    private final int rows;
    private int nextRowToBeReturned = 0;
    @NotNull
    private List<GridTRElement> rowsToBeReturned = new ArrayList<>();

    public PagedGridTRElementIterator(@NotNull GridElement grid) {
        this.grid = grid;
        rows = grid.getRowCount();
    }

    @NotNull
    public static Iterable<GridTRElement> iterable(@NotNull final GridElement grid) {
        return new Iterable<>() {
            @NotNull
            @Override
            public Iterator<GridTRElement> iterator() {
                return new PagedGridTRElementIterator(grid);
            }
        };
    }

    @Override
    public boolean hasNext() {
        return nextRowToBeReturned < rows;
    }

    @Override
    public GridTRElement next() {
        if (!hasNext()) {
            throw new NoSuchElementException();
        }
        if (rowsToBeReturned.isEmpty()) {
            grid.scrollToRow(nextRowToBeReturned);
            int firstVisibleRowIndex = grid.getFirstVisibleRowIndex();
            final int lastVisibleRowIndex = grid.getLastVisibleRowIndex();
            if (nextRowToBeReturned < firstVisibleRowIndex || nextRowToBeReturned > lastVisibleRowIndex) {
                throw new IllegalStateException("Invalid state: the grid scrolled unexpectedly: expected " + nextRowToBeReturned + " but got " + firstVisibleRowIndex + ".." + lastVisibleRowIndex);
            }
            firstVisibleRowIndex = nextRowToBeReturned;
            rowsToBeReturned = grid.getRows(firstVisibleRowIndex, lastVisibleRowIndex);
            if (rowsToBeReturned.isEmpty()) {
                throw new IllegalStateException("Invalid state: grid returned zero rows");
            }
        }
        final GridTRElement result = rowsToBeReturned.get(0);
        Objects.requireNonNull(result);
        rowsToBeReturned.remove(0);
        nextRowToBeReturned++;
        return result;
    }
}

Let me try to prototype an interator which uses these ideas and the solution mentioned by Tatu above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants