feat: Windows Support #429

JeremyEastham · 2022-04-20T19:25:52Z

Summary of Change

Trying to contribute to this project has been a challenge to set up on Windows. The purpose of this PR is to streamline the project setup process for other developers that use Windows on their development machine. I have fixed a number of Windows-specific bugs and added extensive Windows development setup instructions to CONTRIBUTING.md (for both Git Bash and WSL2).

Before starting work on this PR, only 87% of tests were passing when run on Windows via Git Bash. I chose to use Git Bash because it uses the native Windows version of Java at runtime, while still being able to execute the bash scripts in supertokens-root. It was important to me that the native Windows version of Java was used so that I could make sure that all core functionality remained intact when the core was run on a Windows machine. Many of the tests were failing due to bugs in Utils.java, which did not use System.lineSeparator() correctly when mocking config values. Additionally, a few tests compared exception message strings that were not identical cross-platform.

Related Issues

(no related issues)

Test Plan

I have removed a portion of WebserverTest.differentHostNameTest(). This test was failing because the core was not visible to any of my local IP addresses when hosted on localhost due to my firewall settings. Thus, it was not an issue with the code, but instead, an issue with the test. This may be related to how Windows handles localhost addresses, but documentation is sparse on this issue.

The purpose of this PR is not to add functionality, so no tests have been added.

Documentation Changes

(no changes needed)

Checklist for important updates

Changelog has been updated
- If there are any db schema changes, mention those changes clearly
coreDriverInterfaceSupported.json file has been updated (if needed)
pluginInterfaceSupported.json file has been updated (if needed)
Changes to the version if needed
- In build.gradle
Had installed and ran the pre-commit hook
If there are new dependencies that have been added in build.gradle, please make sure to add them in implementationDependencies.json.
Issue this PR against the latest non released version branch.
- To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
- If no such branch exists, then create one from the latest released branch.

Remaining TODOs for this PR

Figure out why "Running the Core (Manually)" instructions are not working
- Rewrite instructions in CONTRIBUTING.md if needed
Merge feat: Add IntelliJ IDEA Run Configurations supertokens-root#8
- Instructions added to CONTRIBUTING.md are dependent on these run configurations existing
Merge feat: Run Core Script, Script Options, Script Consolidation supertokens-root#9
- I changed the test workflow to use ./startTestEnv instead of ./startTestingEnv which is included in this PR

- Add Windows development instructions - Refactor links to the bottom - Fix formatting, grammar, and punctuation

Remove testing connection from local address, since this test could fail depending on firewall configuration

JeremyEastham · 2022-04-20T19:37:53Z

Note:
The "Run tests" action will not succeed until supertokens/supertokens-root#8 is merged due to the absence of the .testEnvRunning file. I have added a check for this file to ensure that developers run ./startTestingEnv or ./startTestingEnv --wait before tests are run.

rishabhpoddar · 2022-04-22T07:27:59Z

@JeremyEastham please also make sure to run the tests with your modifications via github actions so that the failing PR check passes.

JeremyEastham · 2022-04-24T03:34:43Z

Tests are failing now because I renamed ./startTestingEnv to ./startTestEnv in supertokens/supertokens-root#9. Once that PR is merged, tests should pass (they pass on my Windows machine, but I have not tested on Linux yet). The problem is that I changed the call in this PR to ./startTestEnv, which does not exist in supertokens-root master yet.

src/test/java/io/supertokens/test/AuthRecipeTest.java

rishabhpoddar · 2022-04-22T07:26:56Z

src/test/java/io/supertokens/test/WebserverTest.java

-        try {
-            hello(inetAddress.getHostAddress(), "3567");
-            if (!inetAddress.getHostAddress().equals("127.0.0.1")) {
-                fail();
-            }
-        } catch (ConnectException ignored) {
-        }

        Utils.reset();

        Utils.setValueInConfig("host", "\"127.0.0.1\"");
        hello("localhost", "3567");
        hello("127.0.0.1", "3567");
-        try {
-            hello(inetAddress.getHostAddress(), "3567");
-            if (!inetAddress.getHostAddress().equals("127.0.0.1")) {
-                fail();
-            }
-        } catch (ConnectException ignored) {
-        }


Why are we removing these tests cases?

This test was failing because the core was not visible to any of my local IP addresses when hosted on localhost due to my firewall settings. This may also be related to how Windows handles localhost addresses, but documentation is sparse on this issue.

I see. I would require these tests to be there.. Maybe we can change the code here to not run if the OS is windows?

I thought about that, but wouldn't the situation be similar on other operating systems as well? If settings are strict (as they probably should be), access to localhost servers would not be visible to LAN addresses.

It has more or less worked with all linux based systems, mac, CICD, GitHub action and everyone in our dev team.. so i don't think it's too much of a problem. I just want this to also run in CICD / GH action pipeline. It would be OK to have them skipped in local dev env.

src/test/java/io/supertokens/test/Utils.java

rishabhpoddar · 2022-04-27T05:29:25Z

src/test/java/io/supertokens/test/Utils.java

+        String find = "\r?\n#?\\s*" + Pattern.quote(key) + ":.*\r?\n";
+        String replace = newLine + key + ": " + value + newLine;


Do we need to change the regex expression? The older one worked just fine. Did the older one cause issues in windows?

Yes, this change is necessary.

Old Regex: "\n((#\\s)?)" + key + "(:|((:\\s).+))\n" New Regex: "\r?\n#?\\s*" + Pattern.quote(key) + ":.*\r?\n"

Issues with the Old Regex:

It used \n instead of \r?\n, which did not match Windows line endings (\r\n)

It only matched comments with a single whitespace character after it

It did not match non-commented keys with leading whitespace

It was also much harder to read due to unnecessary group nesting and the unnecessary | operator

It did not quote the key, so any regex characters in the key would make it fail

When reviewing this, I actually noticed one issue with the New Regex. It did not match commented lines with leading whitespace. I have committed this change.

Ok. So this change would be required in the plugins repos as well. Since they too have a similar function in there.

Okay, I see. I was thinking that the plugin repos called io.supertokens.test.Utils.commentConfigValue() and io.supertokens.test.Utils.setValueInConfig() directly.

I was also thinking about renaming setValueInConfig() to setConfigValue() to match commentConfigValue(). If I rename all references in supertokens-core, will any change be needed in other repositories?

You should change them in the plugin repos too, just to be consistent.

CONTRIBUTING.md

JeremyEastham added 8 commits April 16, 2022 18:15

Remove race condition in AuthRecipeTest.paginationTest()

907d2ce

Make file operations work cross-platform

78bc0e9

Major update to CONTRIBUTING.md

f6d1c09

- Add Windows development instructions - Refactor links to the bottom - Fix formatting, grammar, and punctuation

Update CHANGELOG.md

40165cf

Fix Utils.setValueInConfig()

07bb149

Fix WebserverTest.differentHostNameTest()

2ee4d6f

Remove testing connection from local address, since this test could fail depending on firewall configuration

Add Project Setup links to CONTRIBUTING.md

d99e0f4

Observe visibility of .testEnvRunning before running tests

3e295ef

This was referenced Apr 22, 2022

feat: Add IntelliJ IDEA Run Configurations supertokens/supertokens-root#8

Merged

Add image of run configurations supertokens/supertokens-logo#5

Merged

Rename startTestingEnv script to startTestEnv

63bc059

Update CONTRIBUTING.md

5f653d2

JeremyEastham marked this pull request as ready for review April 25, 2022 00:52

rishabhpoddar requested changes Apr 25, 2022

View reviewed changes

src/test/java/io/supertokens/test/Utils.java Show resolved Hide resolved

rishabhpoddar requested changes Apr 27, 2022

View reviewed changes

JeremyEastham added 2 commits April 27, 2022 01:18

Update CONTRIBUTING.md

ed1fc78

Update config regexes in Config.java

e597766

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Windows Support #429

feat: Windows Support #429

JeremyEastham commented Apr 20, 2022 •

edited

JeremyEastham commented Apr 20, 2022

rishabhpoddar commented Apr 22, 2022

JeremyEastham commented Apr 24, 2022 •

edited

rishabhpoddar Apr 22, 2022

JeremyEastham Apr 26, 2022

rishabhpoddar Apr 27, 2022

JeremyEastham Apr 28, 2022

rishabhpoddar Apr 28, 2022

rishabhpoddar Apr 27, 2022

JeremyEastham Apr 27, 2022

rishabhpoddar Apr 27, 2022

JeremyEastham Apr 28, 2022

rishabhpoddar Apr 28, 2022

		String find = "\r?\n#?\\s" + Pattern.quote(key) + ":.\r?\n";
		String replace = newLine + key + ": " + value + newLine;

feat: Windows Support #429

Are you sure you want to change the base?

feat: Windows Support #429

Conversation

JeremyEastham commented Apr 20, 2022 • edited

Summary of Change

Related Issues

Test Plan

Documentation Changes

Checklist for important updates

Remaining TODOs for this PR

JeremyEastham commented Apr 20, 2022

rishabhpoddar commented Apr 22, 2022

JeremyEastham commented Apr 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyEastham commented Apr 20, 2022 •

edited

JeremyEastham commented Apr 24, 2022 •

edited