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

tests: normalize multiple keys on a single dict #229

Merged
merged 1 commit into from Sep 27, 2021

Conversation

schopin-pro
Copy link
Contributor

@schopin-pro schopin-pro commented Sep 24, 2021

Description

The YAML normalizer has some special cases for some given keys. However,
the code block where those are dealt with used elif everywhere, which
meant that if a single dict had multiple keys needing to be dealt with,
only the first one would be fixed, and the rest would continue on, still
broken.

This bug is latent in the current codebase, but it showed up in a
patchset I'm working on, where by the game of butterfly effects the
rendered configuration would insist on having proper renderer for each
netdef. This key needs normalizing, but in the test cases where the
netdefs have other problematic properties, for instance the GSM config
in test_cdma_config, those pesky renderer lines would still show up
and screw up the test.

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a small inline suggestion about the pycheckstyle formatting that we should improve.

Also, we do have the skip_generated_yaml_validation parameter to generate() if there are tests that really do not want to play nicely with the normalizer (just FYI).

tests/generator/base.py Outdated Show resolved Hide resolved
tests/generator/base.py Outdated Show resolved Hide resolved
The YAML normalizer has some special cases for some given keys. However,
the code block where those are dealt with used `elif` everywhere, which
meant that if a single dict had multiple keys needing to be dealt with,
only the first one would be fixed, and the rest would continue on, still
broken.

This bug is latent in the current codebase, but it showed up in a
patchset I'm working on, where by the game of butterfly effects the
rendered configuration would insist on having proper renderer for each
netdef. This key needs normalizing, but in the test cases where the
netdefs have other problematic properties, for instance the GSM config
in `test_cdma_config`, those pesky renderer lines would still show up
and screw up the test.

V2:
* Beat pycheckstyle into submission in a much more sensible fashion,
  thanks to @slyon
@schopin-pro
Copy link
Contributor Author

Thanks for the formatting suggestion!

That parameter wouldn't have been right for this, but thanks for the tip, I'll keep it in mind :)

@codecov-commenter
Copy link

Codecov Report

Merging #229 (36cfbcb) into main (17d3848) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #229   +/-   ##
=======================================
  Coverage   99.04%   99.04%           
=======================================
  Files          57       57           
  Lines        9524     9524           
=======================================
  Hits         9433     9433           
  Misses         91       91           
Impacted Files Coverage Δ
tests/generator/base.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17d3848...36cfbcb. Read the comment docs.

@slyon
Copy link
Collaborator

slyon commented Sep 27, 2021

Thanks for cleaning it up. Let's merge this!

@slyon slyon merged commit 85ef928 into canonical:main Sep 27, 2021
slyon pushed a commit that referenced this pull request Nov 24, 2021
Instead of directly writing into the final network state, use a separate
global parser object to store any intermediate data needed during
parsing. This allows us to ensure that the client can only see valid
configurations, as the parser is opaque to the client, who then must
call our functions to import and validate the data into the final
network state.

Since the public API doesn't allow us to pass around some context, we
still store a static object to use in those APIs, but all the internal
functions shouldn't touch any external data.

This PR depends on #229 and #227 and is only a prequel to the proper YAML
parser unification work, which will start with the removal of the current parsing
API, replaced by one based on explicitly allocated states, in conjunction with
the #232 work. To make things easier, the latter PR has been added as a dependency
for this one.

It also picked up a dependency on #234 along the way :)

Only the commits after the merge commit are part of the PR proper, the rest are
from the dependencies.

COMMITS:
* lib: new functions *_clear to safely clear heap-allocated complex structs

We already have the functions to actually free the objects, these
helpers just wrap those to also clear up the *pointer* and nullifying
it.

These functions are going to be used in the parser code to clear
intermediate state, notably on the error paths.

Function names, signature and implementations are heavily inspired by
g_datalist_clear().

V2:
rename the functions to match the new scheme [prefix_]type_action

* parse: clear the cur_addr_options pointer once the parsing is done

Keeping a pointer around after we've transferred ownership of the data
to a netdef is a recipy for disaster, as we have no way of knowing if
we're still responsible for cleaning up the NetplanAddressOptions
object.

* lib: yaml: change the error goto label to err_path

`error` is usually a GError** argument, and the new name is just as
legible.

* lib: write_netplan_conf_full: use an early-return

This allows us to shift the code left, make it easier to read.

* lib: util: netplan_delete_connection: clear the state at the beginning

The function is supposed to work on an already empty state, as it parses
a whole tree and validates it afterwards. We thus clear it explicitly.

This is a latent bug that might become apparent should the validation be
a little more stringent, such as redefinition of already existing
netdefs.

* tests: clear the global state in teardowns to expose inter-test order dependency

Some tests were relying on the state having been cleared in a previous
state, which isn't necessarily the case.

* cli/utils: force a full parse cycle before extracting ids by devtype

While the current approach works for now, it'll break when we separate
parser state and global state, until we do the actual validation.

* lib: use an explicit parser context

Instead of directly writing into the final network state, use a separate
global parser object to store any intermediate data needed during
parsing. This allows us to ensure that the client can only see valid
configurations, as the parser is opaque to the client, who then must
call our functions to import *and validate* the data into the final
network state.

Since the public API doesn't allow us to pass around some context, we
still store a static object to use in those APIs, but all the internal
functions shouldn't touch any external data.

V2:
* Remove stray cur_filename global variable
* Fix some formatting
* Rename 'done' field to 'parsed_defs'
* Reduced the nocov zone

* libnetplan: rewrite netplan_finish_parse using explicit states

The netplan_finish_parse implementation is moved to our ABI attic,
abi_compat.c, and is now a simple wrapper around
netplan_state_import_parser_results, which is the new function used to
import into a Netplan state the results of a parsing operations. As
before, the idea is to maintain a constantly valid Netplan state, so
this function does validation before import.

* lib: expose new functions to generate YAML from Netplan state or defs

The old functions have been rewritten as wrappers around the new APIs

V2:
* Add some header comments to the new functions

* parse-nm: new API using the separate parser state

* lib: add accessors to be able to reach filename from netdef ID

* lib: rewrite process_input_file and process_yaml_hierarchy using the new API scheme

There are no process_input_file() replacement as it can very well be
written by the client code. process_yaml_hierarchy() has been replaced
by a function in utils, netplan_parser_load_yaml_hierarchy(), which
leaves the error policy up to the client code rather than calling
exit().

The old versions have been moved to abi_compat.c and reimplemented to
keep their previous semantics.

* lib: move _write_netplan_conf to the abi_compat.c file

This function is only there for legacy reasons, and should be replaced
by a normalized API.

* lib: netplan_get_filename_by_id: move to abi_compat.c

This is an old API. The ABI compat version is entirely reimplemented
using public, up-to-date functions.

* lib: util: reimplement netplan_delete_connection with the new APIs

This new implementation does away with global state, creating its own
local state instead.

V2: fix formatting

* netplan:utils: mark the exception path of the devtype iter as nocover

* netplan/cli/utils: move the iter ABI check in the wrapper class

This makes it possible to use the wrapper class in autonomy, and remove
the implicit dependency between the netplan_get_ids_for_devtype tests
and the _NetdefIdIterator tests. The latter would crash if run before
the former, as the ctypes bindings wouldn't have been initialized.

* lib: networkd: fix a stray call to an old API

THis call meant that the network file was generated using the global
state as reference instead of the local np_state, making things such as
the VLAN feature basically useless.

* generate: migrate to the new libnetplan APIs

This allows us to do proper cleanup on error cases.

Note that since generate was the only consumer of some of the legacy
APIs, those endpoints are not hit by the testsuite anymore and are thus
marked as ignored by the coverage calculations.
@schopin-pro schopin-pro deleted the test-normalize-fix branch April 13, 2022 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants