search

vaadin / patient-portal-demo-flow

0 stars 0 forks source link

Make UI components and IT tests to measure memory consumption #80

Closed denis-anisimov closed 6 years ago

denis-anisimov commented 6 years ago

Several components are added into the tests source root which output the memory size. It tests reads the memory and compare it with the golden values.

Fix for #77

This change is Reviewable

caalador commented 6 years ago

Due to having IT tests you will need to migrate the build from travis to bender as travis doesn't have the license key and on bender the actual status is more visible.

Reviewed 1 of 15 files at r1. Review status: 1 of 15 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

denis-anisimov commented 6 years ago

Right........

I was going to make a TC configuration to run the tests nightly.

81 is created as a reminder for the IT tests.

I'm not going to enable IT tests here. It's out of the scope of the memory measurement ticket. The verification with IT tests is totally broken at the moment: no single IT test passes. I will disable all of them except the new ones and remove verify goal from the travis script.

Review status: 1 of 15 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

caalador commented 6 years ago

Reviewed 14 of 15 files at r1, 5 of 5 files at r2. Review status: all files reviewed at latest revision, all discussions resolved.

.travis.script.sh, line 118 at r2 (raw file):

    # Validation should not be done via Travis at all:
    # there is no license key for travis
fi

I would rather drop the whole travis and just have bender build any nightlies and PRs for this in the future as this is not a public project.

src/main/java/com/vaadin/flow/demo/patientportal/ui/PatientsView.java, line 108 at r2 (raw file):

        // todo improve the app security
        if (UI.getCurrent().getSession().getAttribute("login") == null) {
            UI.getCurrent().navigate("");

Any reason we drop the reroute and navigate to "" instead of rerouting to loginView and dropping navigate to ""?

Comments from Reviewable

denis-anisimov commented 6 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

.travis.script.sh, line 118 at r2 (raw file):

Previously, caalador wrote…
I would rather drop the whole travis and just have bender build any nightlies and PRs for this in the future as this is not a public project.

We need some validation on each commit: the project should be compilable. Nightlies are not enough for this.

So there can be additional build on bender triggered on each commit but it has no sense at the moment since there are no IT tests which pass. Travis is able to validate compilation. So for the memory measurement task the current config is OK. In the future the validation should be done via bender.

src/main/java/com/vaadin/flow/demo/patientportal/ui/PatientsView.java, line 108 at r2 (raw file):

Previously, caalador wrote…
Any reason we drop the reroute and navigate to "" instead of rerouting to loginView and dropping navigate to ""?

As I understand there is no need to do both. Either one ore another is needed. I remember some difference between Router.navigate() and UI.navigate(). I'm not sure which one is more correct here. At least it works in the way it's written. But actually I don't need this logic and may return back the code.

Comments from Reviewable

caalador commented 6 years ago
:lgtm:

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable