(ASIM-4306) network widgets custom fonts

[dettmerramon]

No description provided.

[nicoddemus]

Can we have a test for this functionality? We need at least some test to use this, to ensure future code changes don't break it unexpectedly.

[haensch-mauricio]

Will wait for a test too.

[dettmerramon]

How would I go about writing a test for it? Any examples?
I wrote one in the alfasim_gui rep but seems it isn't enough, although I do agree that it could benefit from one

[nicoddemus]

I wrote one in the alfasim_gui rep but seems it isn't enough, although I do agree that it could benefit from one

The point is that changes in a library should be accompanied by a test, so if we break the functionality a test in the library will point the breakage.

@prusse-martin any suggestions?

[prusse-martin]

I wrote one in the alfasim_gui rep but seems it isn't enough, although I do agree that it could benefit from one

The point is that changes in a library should be accompanied by a test, so if we break the functionality a test in the library will point the breakage.

An image test (comparing the screen shot).
This will probably be SO dependent. Today we only run the ci tests on linux, but I really like to be able to run them on my local machine (windows).

Another variation of the image test is using a canvas (set font and fill text then extract the pixel data from the canvas). But I will like to keep it simple. If mxgraph is shows to be "flaky" on node/label positioning we can use the canvas approach.

[nicoddemus]

Hmm I think we should move away from anything take needs the UI to work.

Can't we just check that things are set on the JS side, after GraphOptions.custom_fonts has been changed?

[prusse-martin]

Yes we can check that...
But if the font is not properly configured a system default will be used without errors (just the rendering will be wrong).
That is how it is tested in alfasim, checking that "we properly instructed qmx to setup things", I think we should check here that qmx setup is actually working.

I am ready to stand down if you think we should not take this approach in this case.

[nicoddemus]

But if the font is not properly configured a system default will be used without errors (just the rendering will be wrong).

That's fine, I want more of a smoke test to ensure some refactoring just doesn't forward custom_fonts to JS anymore, and no test here breaks because of that.

That is how it is tested in alfasim, checking that "we properly instructed qmx to setup things", I think we should check here that qmx setup is actually working.

Agreed.

I am ready to stand down if you think we should not take this approach in this case.

What? I thought we are in agreement?

[prusse-martin]

That is how it is tested in alfasim, checking that "we properly instructed qmx to setup things", I think we should check here that qmx setup is actually working.

Agreed.

I am ready to stand down if you think we should not take this approach in this case.

What? I thought we are in agreement?

Sorry if was I was not able to convey my idea.
By "that qmx setup is actually working" I intended to express "that qmx setup is actually resulting in the expected effect".

If a test "that qmx setup is happening" was in place (how it was in alfasim) during the qt port that test will happily pass.
If a test "that qmx setup is actually resulting in the expected effect" it will fail.

[nicoddemus]

If a test "that qmx setup is happening" was in place (how it was in alfasim) during the qt port that test will happily pass.

I disagree, we would have catch this error because we would have seen the setup failing in JS (I'm advocating to check things on the JS side, not only on the Python side).

I don't want to introduce screenshot tests or anything like that because we know how flaky/hard to maintain those are, so I think at least we can meet halfway and have a test which checks the integration is happening.

Do you agree, or you strongly believe we should go for a image/screenshot approach?

[prusse-martin]

I disagree, we would have catch this error because we would have seen the setup failing in JS (I'm advocating to check things on the JS side, not only on the Python side).

Test this on JS side would be doing the "canvas" hack, due to security concerns it is not possible to check the actual font used to render in html.
But we could check that in some convoluted (read "canvas hack") ways like suggested here: https://stackoverflow.com/a/38910481/783219
We use as base line an invalid font then check if using a valid font the canvas rendered text changed.

Do you agree, or you strongly believe we should go for a image/screenshot approach?

I believe we should. But I will repeat.
I am ready to stand down if you think we should not take this approach in this case.

[nicoddemus]

OK, let's go with the more complete test then.

Could you tackle that in a separate PR @prusse-martin? I think this is out of @dettmerramon's expertise.

Pushing the test here also works too if you prefer. 👍

[prusse-martin]

I will work on this test later.

[haensch-mauricio]

I will work on this test later.

By later you mean in another PR or in this one?

By the way, tests are failing in this one.

[haensch-mauricio]

Need to check why tests are failing.

[prusse-martin]

I will work on this test later.

By later you mean in another PR or in this one?

Yes. I created a follow up issue for this.

[coveralls]

Coverage increased (+0.01%) to 89.483% when pulling 3d4be6a on fb-ASIM-4306-network-widgets-custom-fonts into 9cbee62 on master.