-
Notifications
You must be signed in to change notification settings - Fork 289
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
🧪 Create Cypress test for Roles of variables #5614
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you can do something like this
response['variables'] = transpile_result.roles_of_variables
response['show_roles'] = show_roles_of_variables()
And then check for that in the front-end
website/auth.py
Outdated
(If not, only variable names and values will be shown in the list). | ||
Checks the configuration in the environment variable $SHOW_ROLES | ||
""" | ||
can_show_roles = os.getenv("SHOW_ROLES") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If SHOW_ROLES
is not set you can also add a default value:
can_show_roles = os.getenv("SHOW_ROLES") | |
can_show_roles = os.getenv("SHOW_ROLES", False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I just noticed this is in auth.py
can you move it to utils.py
?
app.py
Outdated
@@ -2918,6 +2918,7 @@ def on_offline_mode(): | |||
env_defaults = dict( | |||
BASE_URL=f"http://localhost:{config['port']}/", | |||
ADMIN_USER="admin", | |||
SHOW_ROLES=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to change this to "false" or "False" I'm seeing that they require the values of the env variables to be strings.
static/js/debugging.ts
Outdated
@@ -79,6 +79,7 @@ export function load_variables(variables: any) { | |||
if (variables[i][1]) { | |||
const variableName = variables[i][0].replace(/^_/, ''); | |||
const role = programData?.variables[variableName]; | |||
const showRoles = programData?.show_roles; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're having a problem with this, because you need to add program_data
to the list of variables!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already had const role = programData?.variables[variableName]
in the code when I still used the feature flag in the frontend, then things worked fine...
app.py
Outdated
@@ -610,6 +610,7 @@ def parse(): | |||
response['has_sleep'] = True | |||
|
|||
response['variables'] = transpile_result.roles_of_variables | |||
response['show_roles'] = show_roles_of_variables() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's not defined inside app.py
you need to use this function as : utils.show_roles_of_variables()
app.py
Outdated
@@ -2918,6 +2918,7 @@ def on_offline_mode(): | |||
env_defaults = dict( | |||
BASE_URL=f"http://localhost:{config['port']}/", | |||
ADMIN_USER="admin", | |||
SHOW_ROLES=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to change this to "false" or "False" I'm seeing that they require the values of the env variables to be strings.
static/js/debugging.ts
Outdated
@@ -79,6 +79,7 @@ export function load_variables(variables: any) { | |||
if (variables[i][1]) { | |||
const variableName = variables[i][0].replace(/^_/, ''); | |||
const role = programData?.variables[variableName]; | |||
const showRoles = theGlobalDebugger.getProgramData().show_roles; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here we'd need to do something like
const showRoles = theGlobalDebugger.getProgramData().show_roles === "false"
I said in the export function runPythonProgram (file app.ts) that the type of showRoles is a boolean, but now it needs be a string then? I'll just make the type any, to be sure it's okay... |
If you just send the variable it'll be a string, you can do something like:
and that'll always send a boolean |
If I do this, then the boolean will become true when the string is "False" right? I'm confused... |
Yes, you're right! It's PD: I edited your comment by accident, sorry! |
some tests keep failing though... Am I still doing things wrong with the env var? |
The feature flag for showing the roles of variables was set in the frontend, but now it is set as a variable in the environment, so that we can test the feature with a cypress test.
How to test
Follow these steps to verify this PR works as intended:
Checklist
Done? Check if you have it all in place using this list: (mark with x if done)
If you're unsure about any of these, don't hesitate to ask. We're here to help!