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

Fix test login (maybe) #90

Merged
merged 5 commits into from
Apr 4, 2024
Merged

Fix test login (maybe) #90

merged 5 commits into from
Apr 4, 2024

Conversation

solderq35
Copy link
Contributor

@solderq35 solderq35 commented Apr 1, 2024

Issue

Original Error (running yarn test)
image

In addition to the above error:

  • Before, if you logged in as a regular user (running yarn start), didn't log out afterwards, then ran the app again as yarn test, then it tries to log you in as a regular user again instead of test user (due to persistent login code conflicting with the test user code)

Changes (still applicable to latest commit of this PR)

Tweaked auth logic to prevent persisting the regular user login if running in test mode (yarn test).

Other changes to drill input is just what happened from running yarn pretty

Troubleshooting

  • Deleting the contents of node_modules folder (and regenerate them with yarn) may also help with preventing the error, not sure (regardless, the changed code should still be useful so that the test user works without having to log out the real user first).
  • Also try running app normally as yarn start and logging out of regular user
  • Clear yarn cache with yarn start -c or yarn test -c (suggested by Frank)

Other potential features

  • In the future it could be worth just adding a test login button rather than relying on environmental variables if it's easier to debug / more intuituive

Copy link
Contributor

@Gehrkej Gehrkej left a comment

Choose a reason for hiding this comment

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

Going through the troubleshooting options, I wasn't able to get yarn test to work even when deleting and reinstalling node_modules as well as appending a -c when running it. The solution I found that worked was just using yarn start and creating an account. Although with this, I found there was already an account made using my ONID. To reset my password, I navigated to firebase project, went to authentication, found my email and utilized the reset password email functionality.

I believe Jeff's changes look good for preventing persistent login but was unable to fully test as I kept running into the initial issue.

context/Auth.js Outdated Show resolved Hide resolved
Copy link
Contributor

@FrankreedX FrankreedX left a comment

Choose a reason for hiding this comment

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

it's fine

@solderq35
Copy link
Contributor Author

solderq35 commented Apr 3, 2024

Test Login Button Version (backup option, not in latest commit of this PR)

EDIT: this commit is outdated, but you can run it with git checkout 50dbb4d

=== original comment below

Just run the app with yarn start and then you can choose to log in normally ("Login" button), or with test user ("Test Login" button).

I couldn't keep fake user logged in after reloading or restarting the app with this version (no persistent storage), as I was trying to avoid relying on custom environmental files (e.g. env.test) which I think caused the original bug.

Note that this also means that if you log in as real user first, then you need to log out again before you can use the test user version (the real user persists and the test user does not).

An alternative approach could be to just have the developers (us) edit the a flag in the default env.development env file (since env.test seems to cause issues) to switch between "test user" and "real user" login mode, which could easily support persistent login while avoiding the issue of custom env files. But it might be annoying to edit the env file frequently.

image

@solderq35
Copy link
Contributor Author

solderq35 commented Apr 3, 2024

Final Version? (yarn test without error)

This is the final version for latest commit of this PR.

image

  • The actual functionality of the app is unchanged compared to this previous commit

  • The Test Login button (Fix test login (maybe) #90 (comment)) commit has been reverted as I think overall the previous test login with command line arguments was working fine

    • I have kept the Test Login button commit as a backup just in case the latest changes don't work. You can check out the test login button version with git checkout 50dbb4d

Testing

Run the following in terminal

  • git fetch
    • Optional, just to make sure you get all the new branches from remote if applicable
  • git checkout solderq35/persist-async-storage
  • git pull
    • Optional, only if not up to date with remote
  • yarn
  • yarn test -c
    • No need to actually run the app on your phone yet, but let the app finish compiling, then quit with Ctrl / Cmd C
  • git checkout layout
  • git pull
    • Optional, only if not up to date with remote
  • yarn test
    • This time, scan the QR code with your phone, which should confirm if the error triggers or not (...Invalid call at line 2: process.env.EXPO_ROUTER_APP_ROOT... First argument of require.context should be a string denoting the directory to require.)
    • The error should trigger here
  • git checkout solderq35/fix-test-login
  • git pull
    • Optional, only if not up to date with remote
  • yarn test
    • Scan QR code with phone, app should compile this time and log you in as test user without errors

Check my log file below in case I made a mistake in my instructions above
reproduce-build-cache-bug.log

Explanation / Research (summary)

Git Branch differences

  • solderq35/persist-async-storage changes some stuff in app.json to account for persistent caching
    • As explained by Expo dev Evan Bacon in this thread, changes to app.json may cause issues, which might resolved by clearing the bundler cache
      • However, this did not fully explain the bug at least in our use case, because Jake reported that sometimes clearing the cache yarn test -c did not fix the bug, and I also found that sometimes clearing the cache with -c was not enough
        • NOTE: It seemed like another (relatively) fail-safe method was to add or remove some arbitrary package to package.json, then run yarn, then do yarn test -c
          • This is why I intentionally only ran yarn on solderq35/persist-async-storage branch, and not on layout, nor solderq35/fix-test-login branch. Since persist-async-storage branch has package differences compared to layout and fix-test-login branches, running yarn on persist-async-storage along with yarn test -c "reset" the build cache onto the persist-async-storage branch, and caused the build cache to expect the app.json of persist-async-storage (theory)
          • It's also why I made sure that babel-plugin-transform-inline-environment-variables library was in both persist-async-storage and fix-test-login branches, so that the fix-test-login branch would at least compile without needing to run yarn to reinstall packages again
  • The other half of the puzzle was the solution from this other thread to make edits to babel.config.js to resolve issues with expo root / babel, and to add babel-plugin-transform-inline-environment-variables library
    • Since the app compiled on solderq35/fix-test-login branch, and there are still differences in how app.json works compared to solderq35/persist-async-storage branch, this should prove that the changes to the babel config fixed the expo root error without having to add libraries to the package.json / clear build cache
      • It is generally still advised to clear the build cache with -c after making changes to app.json, but the changes in this PR should enable the app to keep working even if you forget to do that
    • For future reference, we seem to still be on Expo SDK 49 "expo": "~49.0.15" in package.json (which is why the first thread I linked from Evan Bacon is about SDK 49), so the babel / expo root / environmental variables stuff may need to be updated if we ever move to Expo SDK 50 or higher

Copy link
Contributor

@FrankreedX FrankreedX left a comment

Choose a reason for hiding this comment

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

sick. I have tested this on my machine and it works

Copy link
Contributor

@Gehrkej Gehrkej left a comment

Choose a reason for hiding this comment

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

Code makes sense for the changes that were made and when testing yarn test -c I was able to get into the app! Great work tracking down this strange bug Jeff!

@FrankreedX FrankreedX merged commit 840e5ee into layout Apr 4, 2024
1 check passed
@FrankreedX FrankreedX deleted the solderq35/fix-test-login branch April 4, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants