Console app on convert by logan-keede · Pull Request #2089 · beeware/briefcase

logan-keede · 2024-12-18T15:58:50Z

Describe your changes in detail

I have added prompt that asks whether the given app is console_app and adds corresponding configuration to pyproject.toml.
also provides corresponding override by the name of console_app

What problem does this change solve?

It addresses the need to manually change pyproject.toml when converting console apps.

If this PR relates to an issue, include Refs #XXX or Fixes #XXX

Fixes #1900

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

…rage_test change

freakboy3742

This is definitely on the right track; two comments inline regarding the implementation.

There's also a need for tests. Briefcase has 100% branch coverage, and any new feature requires tests of that new feature. The Briefcase contribution guide (in the docs) has pointers on how to set up a local testing environment.

If you need any pointers on these points, let us know and I'll do what I can to help.

freakboy3742 · 2024-12-19T01:48:20Z

src/briefcase/commands/convert.py

+        ):
+            return {
+                "gui_framework": "None",
+                "pyproject_table_briefcase_app_extra_content": "\nconsole_app = true\n",


If this is a feature we're going to rely on, we should probably add console_app as a feature in the Briefcase template itself, rather than slipping it into "extra" content.

alright, I will make a commit there first. should I open an issue first?
Thanks for the guidance.

There's no need for a separate issue; you can use Refs beeware/briefcase#1234 syntax to reference an issue or PR in a separate project.

src/briefcase/commands/convert.py

logan-keede · 2024-12-19T13:48:53Z

There's also a need for tests. Briefcase has 100% branch coverage, and any new feature requires tests of that new feature. The Briefcase contribution guide (in the docs) has pointers on how to set up a local testing environment.

Hi, I get this error in coverage test, but I can not completely understand what I am doing wrong. I would be grateful if you can point me in right direction.

My analysis:-
The self.values that is being poped does not seem to be under my control, because all coverage test pass if I set self.input.tool=DummyConsole("") at
conftest.py#L26 which seems to be equivalent of empty input instead of previous no input at all.
This no input at all does not bother other test case since we are using overrides, (like build_app_context) but this particular test is referred as def test_overrides_are_not_used(convert_command): which adds to my confusion.

Possible solution:-

use DummyConsole('') but since input.tools is used by other tests, it may effect coverage quality?
add Overrides in this test(for new feature console_app) and change function name.
other?

Thanks,
logan

freakboy3742 · 2024-12-20T00:53:50Z

Hi, I get this error in coverage test, but I can not completely understand what I am doing wrong. I would be grateful if you can point me in right direction.

The underlying issue is that as a result of the changes you've made, the test in question is now asking for user input. This input is controllable, in multiple ways - see the tests for test_input_* in the same folder.

However, in this case, the test is misleadingly named, and it should be verifying that overrides are being used - so, your option 2.

logan-keede · 2024-12-26T19:53:45Z

I believe I have aptly addressed the requested changes, let me know if I am wrong or more changes are required.
Thanks!!!

freakboy3742

This is getting pretty close; a couple of minor issues flagged inline, several of which are tied to the exact form of the template.

The only additional detail that needs to be modified is the handling of console mode for new apps. At present, the Console bootstrap (briefcase.bootstraps.console) includes console_app = true as a hard-coded string in the pyproject_table_briefcase_app_extra_content configuration. For consistency, this should probably be handled as a boolean flag on the bootstrap itself (by subclassing extra_context()) that will be passed down to the bootstrap template.

freakboy3742 · 2024-12-28T03:27:09Z

tests/commands/convert/test_build_gui_context.py

+    assert out["gui_framework"] == "None"
+    assert overrides["gui_framework"] == "Toga"


This changes the test quite a bit - it goes from checking the entire contents of the dictionary, to checking one specific field in the dictionary. If the method adds additional fields, the test won't raise this as a failure.

src/briefcase/commands/convert.py

changes/1900.feature.rst

freakboy3742 · 2024-12-28T03:31:29Z

src/briefcase/commands/convert.py

+            console_app = "true"
+        else:
+            console_app = "false"


Matching the changes I made to the template - should this be a string, or a boolean? I suspect it should be a boolean. input_console_app is already outputting a boolean.

I think, it should be string since making console_app a boolean renders console_app in pyproject.toml to True instead of true, or is it case insensitive?

TOML parsing is case sensitive; I'd argue this means the template should use {{ "True" if console_app else "False" }}. The template context should be "pure" data; the template then renders that context as necessary to produce valid TOML.

freakboy3742 · 2024-12-28T03:32:28Z

tests/commands/convert/test_input_console_app.py

+def test_overrides_are_used_for_GUI(convert_command):
+    overrides = {"console_app": "GUI"}
+    out = convert_command.build_gui_context({}, overrides)
+    assert out["console_app"] == "false"


There should also be a test of the two "normal" cases, where user input is provided.

from suggested changes Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>

…de/briefcase into console_app_on_convert

freakboy3742

Getting close; most of the issues now stem from the exact value being put into the template context

freakboy3742 · 2024-12-30T00:17:08Z

src/briefcase/commands/convert.py

+            console_app = "true"
+        else:
+            console_app = "false"


TOML parsing is case sensitive; I'd argue this means the template should use {{ "True" if console_app else "False" }}. The template context should be "pure" data; the template then renders that context as necessary to produce valid TOML.

freakboy3742 · 2024-12-30T00:18:09Z

tests/commands/convert/test_input_console_app.py

+        default=None,
+        options=["GUI", "Console"],
+        override_value=None,
+    )


This validates that the method is called; but doesn't validate the return value of the method being invoked.

Right, test_input_is_used_GUI and test_input_is_used_Console are supposed to validate returned value(just below this function), I saw that other input functions had similar test to check if the function is being called properly, so I added one for this one too.

Sure - my point is that this test is incomplete. If you're invoking a function, you should be ensuring that the return value is what you expect.

However, the test itself is actually redundant. We don't actually care if select_option is invoked - that's testing a specific implementation detail. What we care about is the "black box" behavior - that if the user types 1/0 after invoking the method, a False/True value is returned; for bonus points, we can also check the default, and error handling options. Those outcomes are established by the subsequent tests (which can be parameterised to account for the other input options).

freakboy3742 · 2024-12-30T00:20:44Z

tests/commands/convert/test_build_gui_context.py

-    assert out == {"gui_framework": "None"}
-    assert overrides == {"gui_framework": "Toga"}
+    assert out == {"gui_framework": "None", "console_app": "true"}
+    assert overrides["gui_framework"] == "Toga"


I mentioned this in a previous review, but this is changing the nature of the assertion from a full dictionary to a single key.

from suggestions

freakboy3742

Thanks for those updates - other than a couple of minor issues that I've updated manually, this looks great! Thanks for the contribution!

freakboy3742 · 2024-12-31T00:17:19Z

tests/commands/convert/test_input_console_app.py

+        default=None,
+        options=["GUI", "Console"],
+        override_value=None,
+    )


Sure - my point is that this test is incomplete. If you're invoking a function, you should be ensuring that the return value is what you expect.

However, the test itself is actually redundant. We don't actually care if select_option is invoked - that's testing a specific implementation detail. What we care about is the "black box" behavior - that if the user types 1/0 after invoking the method, a False/True value is returned; for bonus points, we can also check the default, and error handling options. Those outcomes are established by the subsequent tests (which can be parameterised to account for the other input options).

freakboy3742 · 2024-12-31T00:17:42Z

src/briefcase/commands/convert.py

        project_overrides: dict[str, str],
    ) -> dict[str, str]:
-        # We must set the GUI-framework to None here since the convert-command uses the new-command
+        # We must set the GUI-framework to None here since the convert-command uses the new-commandcd


Looks like a window focus typo.

freakboy3742

Wait - I just realised I missed something - the handling of console_app in the console app bootstrap. One more small update to come.

logan keede added 4 commits December 18, 2024 20:28

Adding prompt to set console_app option during convert command

f48e547

Adding prompt to set console_app option during convert command + cove…

ab286b3

…rage_test change

Add docstring for new function

5d0c8bd

changelog addition

17819a4

freakboy3742 requested changes Dec 19, 2024

View reviewed changes

logan-keede marked this pull request as draft December 19, 2024 11:22

installer added 2 commits December 27, 2024 00:15

proper console_app addition

95b9e5d

merge

4387635

logan-keede mentioned this pull request Dec 26, 2024

Console app on convert beeware/briefcase-template#169

Merged

4 tasks

installer added 3 commits December 27, 2024 00:40

test modification

d96066f

test for console_app=GUI

9b524c0

better pytest placement

e52713b

logan-keede marked this pull request as ready for review December 26, 2024 19:41

logan-keede requested a review from freakboy3742 December 26, 2024 19:48

freakboy3742 requested changes Dec 28, 2024

View reviewed changes

logan-keede and others added 5 commits December 29, 2024 15:48

Update src/briefcase/commands/convert.py

6105119

from suggested changes Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>

Update changes/1900.feature.rst

9191fde

from suggested changes Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>

test addition and improvement

13818a3

test addition and improvement

4a0a0f6

Merge branch 'console_app_on_convert' of https://github.com/logan-kee…

ee78b95

…de/briefcase into console_app_on_convert

logan-keede requested a review from freakboy3742 December 29, 2024 19:14

freakboy3742 requested changes Dec 30, 2024

View reviewed changes

logan-keede and others added 5 commits December 30, 2024 13:26

Update test_build_gui_context.py

f93d1c2

from suggestions

passing pure data as per suggestion

fed75ce

test modification

c4ac24a

Correct minor typo.

10ccf08

Slight refactor of test cases.

cfa31df

freakboy3742 approved these changes Dec 31, 2024

View reviewed changes

freakboy3742 reviewed Dec 31, 2024

View reviewed changes

Modify handling of console_app in the console bootstrap.

241d056

freakboy3742 merged commit aa1f5d9 into beeware:main Dec 31, 2024
48 checks passed

		assert out["gui_framework"] == "None"
		assert overrides["gui_framework"] == "Toga"

Uh oh!

Comments

Conversation

logan-keede commented Dec 18, 2024 • edited by freakboy3742 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes in detail

What problem does this change solve?

If this PR relates to an issue, include Refs #XXX or Fixes #XXX

PR Checklist:

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

logan-keede commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freakboy3742 commented Dec 20, 2024

Uh oh!

logan-keede commented Dec 26, 2024

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

logan-keede commented Dec 18, 2024 •

edited by freakboy3742

Loading

logan-keede commented Dec 19, 2024 •

edited

Loading