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.

Sign up for GitHub

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

Jump to bottom

Global west state makes it harder to unit test #149

Open
mbolivar opened this issue Jan 10, 2019 · 3 comments
Open

Global west state makes it harder to unit test #149

mbolivar opened this issue Jan 10, 2019 · 3 comments
Labels
enhancement New feature or request
Milestone
v2.0.0

Comments

@mbolivar
Copy link
Contributor

mbolivar commented Jan 10, 2019 •
edited by marc-hb
Loading

West has some global state that makes it harder to write proper unit tests. Examples:

  • global configuration state is written into a module-level object in west.config
  • log verbosity level is too

This makes it harder to write proper unit tests and causes other annoyances.

2024 update:

We're down to west.log at this point. We can close this issue once that deprecated module is removed sometime in the west 2.0 or later timeline.
@mbolivar mbolivar self-assigned this Jan 10, 2019
@mbolivar mbolivar added bug Something isn't working priority: low labels Jan 10, 2019
@mbolivar mbolivar mentioned this issue Jan 29, 2019
Closed
@mbolivar mbolivar added enhancement New feature or request and removed bug Something isn't working labels Jan 29, 2019
@mbolivar
Copy link
Contributor Author

This bugs me all the time when I write tests, and I'm constantly having to hack around it, as shown in #271. Bumping priority to medium as a result (since it's a problem that is slowing down development, despite not being user-visible).

mbolivar pushed a commit to mbolivar/west that referenced this issue Nov 5, 2019
@mbolivar-nordic
main.py: re-work, adding a new WestApp class
59de0ec 
The main() function has gotten really long, and it's getting hard to
read. I want to break it up, but there's a fair bit of interdependent
state to manage.

Rather than using global variables, create a WestApp instance to
contain them all and use it instead. This helps with readability
without making zephyrproject-rtos#149 harder to do.

We'll extend this later to handle manifest imports.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar pushed a commit to mbolivar/west that referenced this issue Nov 15, 2019
@mbolivar-nordic
main.py: re-work, adding a new WestApp class
5be7cde 
The main() function has gotten really long, and it's getting hard to
read. I want to break it up, but there's a fair bit of interdependent
state to manage.

Rather than using global variables, create a WestApp instance to
contain them all and use it instead. This helps with readability
without making zephyrproject-rtos#149 harder to do.

We'll extend this later to handle manifest imports.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar pushed a commit to mbolivar/west that referenced this issue Nov 16, 2019
@mbolivar-nordic
main.py: re-work, adding a new WestApp class
3d6f94c 
The main() function has gotten really long, and it's getting hard to
read. I want to break it up, but there's a fair bit of interdependent
state to manage.

Rather than using global variables, create a WestApp instance to
contain them all and use it instead. This helps with readability
without making zephyrproject-rtos#149 harder to do.

We'll extend this later to handle manifest imports.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar pushed a commit to mbolivar/west that referenced this issue Nov 16, 2019
@mbolivar-nordic
main.py: re-work, adding a new WestApp class
974da56 
The main() function has gotten really long, and it's getting hard to
read. I want to break it up, but there's a fair bit of interdependent
state to manage.

Rather than using global variables, create a WestApp instance to
contain them all and use it instead. This helps with readability
without making zephyrproject-rtos#149 harder to do.

We'll extend this later to handle manifest imports.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar pushed a commit to mbolivar/west that referenced this issue Nov 19, 2019
@mbolivar-nordic
main.py: re-work, adding a new WestApp class
73ff22d 
The main() function has gotten really long, and it's getting hard to
read. I want to break it up, but there's a fair bit of interdependent
state to manage.

Rather than using global variables, create a WestApp instance to
contain them all and use it instead. This helps with readability
without making zephyrproject-rtos#149 harder to do.

We'll extend this later to handle manifest imports.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar pushed a commit to mbolivar/west that referenced this issue Dec 4, 2019
@mbolivar-nordic
main.py: re-work, adding a new WestApp class
a442d2f 
The main() function has gotten really long, and it's getting hard to
read. I want to break it up, but there's a fair bit of interdependent
state to manage.

Rather than using global variables, create a WestApp instance to
contain them all and use it instead. This helps with readability
without making zephyrproject-rtos#149 harder to do.

We'll extend this later to handle manifest imports.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
carlescufi pushed a commit that referenced this issue Dec 4, 2019
@mbolivar-nordic @carlescufi
main.py: re-work, adding a new WestApp class
885828e 
The main() function has gotten really long, and it's getting hard to
read. I want to break it up, but there's a fair bit of interdependent
state to manage.

Rather than using global variables, create a WestApp instance to
contain them all and use it instead. This helps with readability
without making #149 harder to do.

We'll extend this later to handle manifest imports.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar pushed a commit to mbolivar/west that referenced this issue Jan 22, 2020
@mbolivar-nordic
tree-wide: move CLI into new west.app package
ef7396b 
This relates to issue zephyrproject-rtos#38, the TL;DR of which is that we've never
gotten around to properly separating west's application internals from
its API in source code, and instead relied on documentation to spell
out exactly what the API was that users could rely on.

Let's start fixing that by moving everything users can't rely on into
a new west.app. This includes everything in west.commands except the
__init__ module, so we can just make that a new src/west/commands.py
file and have it be a module instead of a package. This lets its
pykwalify schema file, west-commands-schema.yml, sit next to it in
src/west, which is flatter than before.

The code changes in this commit (source lines changed, rather than
files just getting moved around) are:

- change the entry point in setup.py to west.app.main:main
- change some imports in src/west/app/main.py to import from
  west.app instead of west.commands
- add a new src/west/app/__init__.py, since we're not using
  namespace packages
- changes in MANIFEST.in and test_help.py to reflect new paths
- adjust some comments and docstrings

This change makes the API divide clearer. This in turn exposes some
problems with our use of west.log from west.manifest:

1. logging to stdout is a bad practice from a library
2. west.log also uses global state (which relates to zephyrproject-rtos#149 also)
   which non-command-line users won't have set up properly
   (this is an example of why zephyrproject-rtos#1 is true)

Subsequent commits will move to only using west.log from within
west.app.* and the existing deprecated west.build and west.cmake APIs,
which users should be migrating away from anyway.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@mbolivar mbolivar mentioned this issue Jan 22, 2020
Merged
mbolivar pushed a commit that referenced this issue Jan 22, 2020
@mbolivar-nordic
tree-wide: move CLI into new west.app package
a58261f 
This relates to issue #38, the TL;DR of which is that we've never
gotten around to properly separating west's application internals from
its API in source code, and instead relied on documentation to spell
out exactly what the API was that users could rely on.

Let's start fixing that by moving everything users can't rely on into
a new west.app. This includes everything in west.commands except the
__init__ module, so we can just make that a new src/west/commands.py
file and have it be a module instead of a package. This lets its
pykwalify schema file, west-commands-schema.yml, sit next to it in
src/west, which is flatter than before.

The code changes in this commit (source lines changed, rather than
files just getting moved around) are:

- change the entry point in setup.py to west.app.main:main
- change some imports in src/west/app/main.py to import from
  west.app instead of west.commands
- add a new src/west/app/__init__.py, since we're not using
  namespace packages
- changes in MANIFEST.in and test_help.py to reflect new paths
- adjust some comments and docstrings

This change makes the API divide clearer. This in turn exposes some
problems with our use of west.log from west.manifest:

1. logging to stdout is a bad practice from a library
2. west.log also uses global state (which relates to #149 also)
   which non-command-line users won't have set up properly
   (this is an example of why #1 is true)

Subsequent commits will move to only using west.log from within
west.app.* and the existing deprecated west.build and west.cmake APIs,
which users should be migrating away from anyway.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@mbolivar-nordic mbolivar-nordic mentioned this issue Jun 10, 2020
Merged
@mbolivar-nordic mbolivar-nordic mentioned this issue Sep 4, 2020
Open
@mbolivar-nordic mbolivar-nordic mentioned this issue Dec 16, 2020
Merged
@mbolivar-nordic mbolivar-nordic mentioned this issue Apr 7, 2021
Merged
mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Mar 10, 2022
@mbolivar-nordic
configuration: add Configuration class
17d3c2b 
This is a high-level class that lets you load and interact with the
configuration files in an object-oriented way. It's similar to how
west.manifest.Manifest lets you interact with the manifest, but for
configuration files.

The new API uses 'config.get("some.option")' style methods instead of
the configparser style 'config.get("some", "option")' with a separated
section and key. This makes the code match the options as they are
documented, making it easier to read and grep for.

Having an object around will allow us to deprecate the current
implementation, which relies on global state. It will also make it
easier to override default configuration behavior in certain
circumstances that will be useful in later patches.

Part of the road towards resolving zephyrproject-rtos#149 and at least one other issue.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@mbolivar-nordic mbolivar-nordic mentioned this issue Mar 10, 2022
Merged
mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Mar 10, 2022
@mbolivar-nordic
configuration: add Configuration class
933ab5e 
This is a high-level class that lets you load and interact with the
configuration files in an object-oriented way. It's similar to how
west.manifest.Manifest lets you interact with the manifest, but for
configuration files.

The new API uses 'config.get("some.option")' style methods instead of
the configparser style 'config.get("some", "option")' with a separated
section and key. This makes the code match the options as they are
documented, making it easier to read and grep for.

Having an object around will allow us to deprecate the current
implementation, which relies on global state. It will also make it
easier to override default configuration behavior in certain
circumstances that will be useful in later patches.

Part of the road towards resolving zephyrproject-rtos#149 and at least one other issue.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Mar 10, 2022
@mbolivar-nordic
configuration: add Configuration class
7f42068 
This is a high-level class that lets you load and interact with the
configuration files in an object-oriented way. It's similar to how
west.manifest.Manifest lets you interact with the manifest, but for
configuration files.

The new API uses 'config.get("some.option")' style methods instead of
the configparser style 'config.get("some", "option")' with a separated
section and key. This makes the code match the options as they are
documented, making it easier to read and grep for.

Having an object around will allow us to deprecate the current
implementation, which relies on global state. It will also make it
easier to override default configuration behavior in certain
circumstances that will be useful in later patches.

Part of the road towards resolving zephyrproject-rtos#149 and at least one other issue.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Mar 18, 2022
@mbolivar-nordic
configuration: add Configuration class
7330c96 
This is a high-level class that lets you load and interact with the
configuration files in an object-oriented way. It's similar to how
west.manifest.Manifest lets you interact with the manifest, but for
configuration files.

The new API uses 'config.get("some.option")' style methods instead of
the configparser style 'config.get("some", "option")' with a separated
section and key. This makes the code match the options as they are
documented, making it easier to read and grep for.

Having an object around will allow us to deprecate the current
implementation, which relies on global state. It will also make it
easier to override default configuration behavior in certain
circumstances that will be useful in later patches.

Part of the road towards resolving zephyrproject-rtos#149 and at least one other issue.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar-nordic added a commit that referenced this issue Mar 18, 2022
@mbolivar-nordic
configuration: add Configuration class
c757d66 
This is a high-level class that lets you load and interact with the
configuration files in an object-oriented way. It's similar to how
west.manifest.Manifest lets you interact with the manifest, but for
configuration files.

The new API uses 'config.get("some.option")' style methods instead of
the configparser style 'config.get("some", "option")' with a separated
section and key. This makes the code match the options as they are
documented, making it easier to read and grep for.

Having an object around will allow us to deprecate the current
implementation, which relies on global state. It will also make it
easier to override default configuration behavior in certain
circumstances that will be useful in later patches.

Part of the road towards resolving #149 and at least one other issue.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@mbolivar-nordic mbolivar-nordic mentioned this issue Aug 30, 2022
Merged
mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Aug 30, 2022
@mbolivar-nordic
commands: add output helpers to WestCommand
82fb3a6 
This is basically a duplicate of the west.log interface, except with:

- better names
- no use of global state
- support for a 'quiet' mode

The goal is to move over built-in commands to using this interface in
order to eliminate global state. That in turn may help us with our
goals of running more west tests in parallel and is just better design
anyway since the west APIs should be clean when used as a library
outside of any command.

We can move over extension functions in zephyr etc. over time as well,
probably also by using a helper that can detect older versions of west
and work anyway.

That will then allow us to deprecate west.log, removing it in the long
term, and adding support for a global "west --quiet <command> args"
style of invocation for the folks who just hate terminal output. I
want to get this conversion all done by the time Zephyr LTS3 is done,
so it's time to get the ball rolling now. I missed the boat on LTS2
and that makes me sad.

Part of: zephyrproject-rtos#149

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar-nordic added a commit that referenced this issue Aug 31, 2022
@mbolivar-nordic
commands: add output helpers to WestCommand
b4d0b38 
This is basically a duplicate of the west.log interface, except with:

- better names
- no use of global state
- support for a 'quiet' mode

The goal is to move over built-in commands to using this interface in
order to eliminate global state. That in turn may help us with our
goals of running more west tests in parallel and is just better design
anyway since the west APIs should be clean when used as a library
outside of any command.

We can move over extension functions in zephyr etc. over time as well,
probably also by using a helper that can detect older versions of west
and work anyway.

That will then allow us to deprecate west.log, removing it in the long
term, and adding support for a global "west --quiet <command> args"
style of invocation for the folks who just hate terminal output. I
want to get this conversion all done by the time Zephyr LTS3 is done,
so it's time to get the ball rolling now. I missed the boat on LTS2
and that makes me sad.

Part of: #149

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@mbolivar-nordic
Copy link
Contributor

We're down to west.log at this point. We can close this issue once that deprecated module is removed sometime in the west 2.0 or later timeline.

@marc-hb marc-hb mentioned this issue Aug 27, 2024
Closed
@pdgendt pdgendt added this to the v2.0.0 milestone Sep 27, 2024
@marc-hb
Copy link
Collaborator

marc-hb commented Oct 10, 2024

Progress from @pdgendt in zephyrproject-rtos/zephyr#79171 and others.

marc-hb added a commit to marc-hb/zephyr that referenced this issue Oct 10, 2024
@marc-hb
west: promote config_get*() functions from sign.py to the base class
c7e25a7 
These two functions have stood the test of the time and they have
absolutely nothing specific to sign.py

This has the benefit of transitioning away from west's global and
deprecated logging interface
(zephyrproject-rtos/west#149) and this
deprecation is what prompted this commit: see zephyrproject-rtos#79240.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb mentioned this issue Oct 10, 2024
Merged
nashif pushed a commit to zephyrproject-rtos/zephyr that referenced this issue Oct 15, 2024
@marc-hb @nashif
west: promote config_get*() functions from sign.py to the base class
8b859ce 
These two functions have stood the test of the time and they have
absolutely nothing specific to sign.py

This has the benefit of transitioning away from west's global and
deprecated logging interface
(zephyrproject-rtos/west#149) and this
deprecation is what prompted this commit: see #79240.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
coreboot-org-bot pushed a commit to coreboot/zephyr-cros that referenced this issue Oct 21, 2024
@marc-hb
west: promote config_get*() functions from sign.py to the base class
87f8a7b 
These two functions have stood the test of the time and they have
absolutely nothing specific to sign.py

This has the benefit of transitioning away from west's global and
deprecated logging interface
(zephyrproject-rtos/west#149) and this
deprecation is what prompted this commit: see #79240.

(cherry picked from commit 8b859ce)

Original-Signed-off-by: Marc Herbert <marc.herbert@intel.com>
GitOrigin-RevId: 8b859ce
Cr-Build-Id: 8733565196572304993
Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8733565196572304993
Copybot-Job-Name: zephyr-main-copybot-downstream
Change-Id: I6fa289130107f52f5b984d4016380f3cf2544002
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5934670
Reviewed-by: Jeremy Bettis <jbettis@chromium.org>
Reviewed-by: Dawid Niedźwiecki <dawidn@google.com>
Commit-Queue: Dawid Niedźwiecki <dawidn@google.com>
Tested-by: Dawid Niedźwiecki <dawidn@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v2.0.0
Development

No branches or pull requests
4 participants
@mbolivar @pdgendt @marc-hb @mbolivar-nordic