Add unittests #29

strump · 2024-11-19T07:30:12Z

strump commented

2024-11-19 07:30:12 +00:00

Phase 1: cover kothic with unittests
Phase 2: add type annotations to arguments and fields, also migrate to f"" strings
Phase 3: small refactoring, no new features
Phase 4: ???

Here I'm covering all base classes Rule, Condition, MapCSS, Eval, libkomwm.py with unit-tests to make sure nothing will brake on new changes.

Work in progress.

* Phase 1: cover `kothic` with unittests * Phase 2: add type annotations to arguments and fields, also migrate to `f""` strings * Phase 3: small refactoring, no new features * Phase 4: ??? Here I'm covering all base classes `Rule`, `Condition`, `MapCSS`, `Eval`, `libkomwm.py` with unit-tests to make sure nothing will brake on new changes. Work in progress.

strump reviewed 2024-11-19 07:31:53 +00:00

src/mapcss/Condition.py

					
				@ -26,3 +26,3 @@

				        if typez == "regex":

				            self.regex = re.compile(self.params[0], re.I)

				            self.regex = re.compile(self.params[1], re.I)

strump commented

2024-11-19 07:31:53 +00:00

Here I fixed bug with regex conditions: [natural=~/water.+/]. Organic Maps never used such conditions in MapCSS styles.
Should we remove regex conditions completely from the kothic?

Here I fixed bug with regex conditions: `[natural=~/water.+/]`. Organic Maps never used such conditions in MapCSS styles. Should we remove regex conditions completely from the `kothic`?

strump reviewed 2024-11-20 06:58:51 +00:00

src/mapcss/Eval.py

					
				@ -18,2 +20,4 @@

				logger = logging.getLogger('mapcss.Eval')

				logger.setLevel(logging.ERROR)

				class Eval():

strump commented

2024-11-20 06:58:51 +00:00

This class is responsible for eval(...) expressions in .mapcss files. But Organic Maps never used evals in styles.
Should we remove eval completely from the project?

This class is responsible for `eval(...)` expressions in .mapcss files. But Organic Maps never used evals in styles. Should we remove eval completely from the project?

strump reviewed 2024-11-28 11:50:43 +00:00

src/libkomwm.py

strump commented

2024-11-28 11:50:39 +00:00

Looks like order of lines in types.txt matters. And we add mapswithme lines for missing types.

Is it correct?

Looks like order of lines in `types.txt` matters. And we add `mapswithme` lines for missing types. Is it correct?

pastk reviewed 2024-11-28 15:45:15 +00:00

src/mapcss/Eval.py

					
				@ -18,2 +20,4 @@

				logger = logging.getLogger('mapcss.Eval')

				logger.setLevel(logging.ERROR)

				class Eval():

pastk commented

2024-11-28 15:45:15 +00:00

Context:

I think remove it!

Context: - https://github.com/organicmaps/kothic/pull/5 - https://github.com/organicmaps/organicmaps/pull/4293 I think remove it!

pastk reviewed 2024-11-28 15:46:49 +00:00

src/libkomwm.py

pastk commented

2024-11-28 15:46:49 +00:00

Yeap order is important and is the same as type's ID

pastk reviewed 2024-11-28 20:31:50 +00:00

src/libkomwm.py

pastk commented

2024-11-28 20:11:12 +00:00

we specify colors in *.mapcss via hex notation, aren't we?

though other notations are supported also, e.g. color: red;

we specify colors in *.mapcss via hex notation, aren't we? though other notations are supported also, e.g. `color: red;`

src/libkomwm.py

pastk commented

2024-11-28 20:20:55 +00:00

maybe visibility.txt is not needed at all... it seems to be used to cache visibility values so that its faster to load by core, but maybe its as fast to get them from protobuf style files which are loaded anyway;
classificator.txt is also used to speed up loading of types hierarchy, but maybe it could be derived from types names as fast..

maybe `visibility.txt` is not needed at all... it seems to be used to cache visibility values so that its faster to load by core, but maybe its as fast to get them from protobuf style files which are loaded anyway; `classificator.txt` is also used to speed up loading of types hierarchy, but maybe it could be derived from types names as fast..

src/mapcss/__init__.py

pastk commented

2024-11-28 20:27:43 +00:00

it'd better to drop this superfluous syntax altogether, please see a discussion in https://github.com/organicmaps/organicmaps/issues/4137#issuecomment-1403638671

but there are some tricky cases that rely on the current unobvious inheritance behavior of area/line/etc - needs to be fixed in mapcss files

it'd better to drop this superfluous syntax altogether, please see a discussion in https://github.com/organicmaps/organicmaps/issues/4137#issuecomment-1403638671 but there are some tricky cases that rely on the current unobvious inheritance behavior of area/line/etc - needs to be fixed in mapcss files

pastk commented

2024-11-28 20:37:33 +00:00

As discussed in the chat there are many things that need to be conceptually improved here which means behavior changes and mapcss syntax changes etc., not just technical refactoring for the sake of readability etc. So I'd be reluctant to spend more time on technical perfection of the code now (some cosmetic changes are fine), as the code is likely to change a lot.

pastk commented

2024-11-28 20:47:40 +00:00

So I'd be reluctant to spend more time on technical perfection of the code now (some cosmetic changes are fine), as the code is likely to change a lot.

But removing of unused code (evals, regex conditions, etc.) is good!

> So I'd be reluctant to spend more time on technical perfection of the code now (some cosmetic changes are fine), as the code is likely to change a lot. But removing of unused code (evals, regex conditions, etc.) is good!

pastk reviewed 2024-11-28 20:54:56 +00:00

tests/testCondition.py

					
				@ -0,0 +240,4 @@

				        self.assertFalse(cond.test({"tunnel": "yes"}))

				    def test_parser_invTrue(self):

				        """ Test conditions in format [!some_tag?] It works the same way as [some_tag != yes]

pastk commented

2024-11-28 20:54:56 +00:00

we use either [some_tag?] (which means any value except no)
or [!some_tag] which means absence or =no

[!some_tag?] is very strange :)

we use either [some_tag?] (which means any value except `no`) or [!some_tag] which means absence or =no [!some_tag?] is very strange :)

pastk reviewed 2024-11-28 21:15:43 +00:00

pastk left a comment

The unit tests look good at a glance!

(with a gotcha that some of them would be obsolete after migration of *.mapcss to internal OM types instead of pseudo-tags)

The unit tests look good at a glance! (with a gotcha that some of them would be obsolete after migration of *.mapcss to internal OM types instead of pseudo-tags)

tests/testCondition.py

					
				@ -0,0 +278,4 @@

				        self.assertFalse(cond.test({"bridge": "maybe"}))

				    def test_untrue(self):

				        """ parseCondition(...) doesn't support this type of condition.

pastk commented

2024-11-28 20:57:52 +00:00

I don't see any use for it.

pastk commented

2024-11-28 21:19:11 +00:00

Re integration tests. Prob a proper full integration test should contain resulting .bin and .txt files to compare with?

strump reviewed 2024-11-29 07:47:43 +00:00

src/libkomwm.py

strump commented

2024-11-29 07:47:43 +00:00

Ok, removed TODO comment

strump reviewed 2024-11-29 07:53:08 +00:00

tests/testCondition.py

					
				@ -0,0 +240,4 @@

				        self.assertFalse(cond.test({"tunnel": "yes"}))

				    def test_parser_invTrue(self):

				        """ Test conditions in format [!some_tag?] It works the same way as [some_tag != yes]

strump commented

2024-11-29 07:53:08 +00:00

I mentioned in comment that this type of conditions is never used in OrganicMaps styles. Another candidate for removal.

strump reviewed 2024-11-29 07:53:43 +00:00

tests/testCondition.py

					
				@ -0,0 +278,4 @@

				        self.assertFalse(cond.test({"bridge": "maybe"}))

				    def test_untrue(self):

				        """ parseCondition(...) doesn't support this type of condition.

strump commented

2024-11-29 07:53:43 +00:00

Another candidate for removal.

strump commented

2024-11-29 08:03:05 +00:00

@pastk previously I had full copy of organicmaps/data/styles/default/light style (with simplified directories structure). I removed that redundant copy and left only tests/assets/case-2-generate-drules-mini with subset of styles. It contains only highways, has 148 types, zoom levels 0-10 and generates only 40 drules. We even can check generated output by hand.

I don't think it makes sense to run full drules generation for all 6 styles to verify output against some reference, because styles in main repo are changed irregularly, so refence data should be changed too.

@pastk previously I had full copy of `organicmaps/data/styles/default/light` style (with simplified directories structure). I removed that redundant copy and left only `tests/assets/case-2-generate-drules-mini` with subset of styles. It contains only highways, has 148 types, zoom levels 0-10 and generates only 40 drules. We even can check generated output by hand. I don't think it makes sense to run full drules generation for all 6 styles to verify output against some reference, because styles in main repo are changed irregularly, so refence data should be changed too.

👍 1

strump commented

2024-11-29 08:07:51 +00:00

As discussed in the chat there are many things that need to be conceptually improved here which means behavior changes and mapcss syntax changes etc., not just technical refactoring for the sake of readability etc. So I'd be reluctant to spend more time on technical perfection of the code now (some cosmetic changes are fine), as the code is likely to change a lot.

I think we should refactor existing code to break libkomwm.py in smaller parts in order to understand which parts we are going to remove completly and which parts should be improved (e.g. MapCSS parsing, generation of types.txt, colors.txt, etc. should stay).

> As discussed in the chat there are many things that need to be conceptually improved here which means behavior changes and mapcss syntax changes etc., not just technical refactoring for the sake of readability etc. So I'd be reluctant to spend more time on technical perfection of the code now (some cosmetic changes are fine), as the code is likely to change a lot. I think we should refactor existing code to break `libkomwm.py` in smaller parts in order to understand which parts we are going to remove completly and which parts should be improved (e.g. MapCSS parsing, generation of types.txt, colors.txt, etc. should stay).

pastk commented

2024-11-29 08:52:52 +00:00

I think we should refactor existing code to break libkomwm.py in smaller parts in order to understand which parts we are going to remove completly and which parts should be improved (e.g. MapCSS parsing, generation of types.txt, colors.txt, etc. should stay).

Maybe I can finish my internal OM types migration PR first then? Let me check its status first and I'll come back with some estimate..

> > I think we should refactor existing code to break `libkomwm.py` in smaller parts in order to understand which parts we are going to remove completly and which parts should be improved (e.g. MapCSS parsing, generation of types.txt, colors.txt, etc. should stay). Maybe I can finish my internal OM types migration PR first then? Let me check its status first and I'll come back with some estimate..

👍 1

pastk commented

2024-12-03 16:52:58 +00:00

Is it ready for review? Not draft anymore?

pastk reviewed 2024-12-10 14:35:50 +00:00

src/mapcss/Condition.py

					
				@ -26,3 +26,3 @@

				        if typez == "regex":

				            self.regex = re.compile(self.params[0], re.I)

				            self.regex = re.compile(self.params[1], re.I)

pastk commented

2024-12-10 14:35:49 +00:00

yeap there is no need for them

pastk reviewed 2024-12-10 14:41:16 +00:00

src/libkomwm.py

pastk commented

2024-12-10 14:41:16 +00:00

@strump could you comment here please?

It seems to me that color = whatever_to_hex() is used here in case a color isn't defined in hex format already, its not necessary float.

Maybe its redundant to require the mapcss.webcolors lib indeed. We use hex notation almost everywhere in styles. Sometimes could be useful to e.g. use simple names like red, green, etc. But not necessary.

@strump could you comment here please? It seems to me that `color = whatever_to_hex()` is used here in case a color isn't defined in hex format already, its not necessary float. Maybe its redundant to require the mapcss.webcolors lib indeed. We use hex notation almost everywhere in styles. Sometimes could be useful to e.g. use simple names like red, green, etc. But not necessary.

pastk commented

2024-12-10 15:03:52 +00:00

There are some warnings when running tests, e.g.

kothic$ python3 -m unittest discover -s tests
........................../home/pastk/dev/OM/organicmaps/tools/kothic/src/libkomwm.py:502: ResourceWarning: unclosed file <_io.TextIOWrapper name='/home/pastk/dev/OM/organicmaps/tools/kothic/tests/assets/case-2-generate-drules-mini/mapcss-mapping.csv' mode='r' encoding='UTF-8'>
  for row in csv.reader(open(os.path.join(ddir, 'mapcss-mapping.csv')), delimiter=';'):
ResourceWarning: Enable tracemalloc to get the object allocation traceback

There are some warnings when running tests, e.g. ``` kothic$ python3 -m unittest discover -s tests ........................../home/pastk/dev/OM/organicmaps/tools/kothic/src/libkomwm.py:502: ResourceWarning: unclosed file <_io.TextIOWrapper name='/home/pastk/dev/OM/organicmaps/tools/kothic/tests/assets/case-2-generate-drules-mini/mapcss-mapping.csv' mode='r' encoding='UTF-8'> for row in csv.reader(open(os.path.join(ddir, 'mapcss-mapping.csv')), delimiter=';'): ResourceWarning: Enable tracemalloc to get the object allocation traceback ```

pastk reviewed 2024-12-10 15:10:05 +00:00

README.md

pastk commented

2024-12-10 15:10:05 +00:00

cd integration-tests
python3 full_drules_gen.py -d ../../../data -o drules --txt

also consider making full_drules_gen.py executable and add a shebang

```suggestion cd integration-tests python3 full_drules_gen.py -d ../../../data -o drules --txt ``` also consider making `full_drules_gen.py` executable and add a shebang

pastk reviewed 2024-12-10 15:10:20 +00:00

README.md

pastk commented

2024-12-10 15:10:20 +00:00

add a trailing newline please

strump reviewed 2024-12-12 16:15:23 +00:00

README.md

strump commented

2024-12-12 16:15:23 +00:00

Updated README.md

strump reviewed 2024-12-12 16:15:41 +00:00

README.md

strump commented

2024-12-12 16:15:41 +00:00

Added newline

strump commented

2024-12-12 16:16:53 +00:00

@pastk I fixed un-closed files in multiple places. This should fix ResourceWarning: unclosed file in the log.

@pastk I fixed un-closed files in multiple places. This should fix `ResourceWarning: unclosed file` in the log.

👍 1

pastk reviewed 2024-12-12 17:27:33 +00:00

README.md

pastk commented

2024-12-12 17:27:33 +00:00

Could you please update the actual command to python3 full_drules_gen.py -d ../../../data -o drules --txt too?

99% of cases the /path/to/organicmaps/data would be fixed as kothic is in tools/kothic/

Could you please update the actual command to `python3 full_drules_gen.py -d ../../../data -o drules --txt` too? 99% of cases the `/path/to/organicmaps/data` would be fixed as kothic is in `tools/kothic/`