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 findTestModule on Mocha Test #2027

Merged
merged 7 commits into from
Sep 16, 2019
Merged

Fix findTestModule on Mocha Test #2027

merged 7 commits into from
Sep 16, 2019

Conversation

ammein
Copy link
Contributor

@ammein ammein commented Sep 13, 2019

The issue on findTestModule is resolved by using a different type of regex :

  • Windows's file path using .match() regex appears to must-have /\\\\file\\\\to\\\\path/g double backward slash that is equal to \\file\\to\\path.

I hope this issue will raise to any file path matching using regex should follow the same format as above stated.

@boutell
Copy link
Member

boutell commented Sep 13, 2019

Thank you!

@boutell
Copy link
Member

boutell commented Sep 13, 2019

Actually, for readability, I suggest that you go back to using regex literals. You don't need the RegExp constructor. You can still have an if statement that assigns one of two regexes:

if (foo) {
  regex = /something/;
} else {
  regex = /something-else/;
}

@boutell
Copy link
Member

boutell commented Sep 13, 2019

Also please run "npx eslint ." and fix any coding standards issues, something is complaining in the CI build (:

@boutell
Copy link
Member

boutell commented Sep 13, 2019

(Tip: npx eslint . --fix fixes most of them for you)

@ammein
Copy link
Contributor Author

ammein commented Sep 13, 2019

Sure, I'll do that 👌

@ammein
Copy link
Contributor Author

ammein commented Sep 14, 2019

Hi @boutell , I push the latest fixes for you.

@ammein
Copy link
Contributor Author

ammein commented Sep 14, 2019

Wait, it seems to be not resolved since other regex taking place on testDir.replace() . I'll have a full test again and push when everything is running smoothly.

…r fs.symlinkSync that uses type of 'junction' that are based on Permission Issue on Windows 10
@ammein
Copy link
Contributor Author

ammein commented Sep 14, 2019

Done. I think this is a major fix whereby Windows 10 produce more WTF behavior. Especially on permissions issue , I need to change the type on the third argument of fs.symlinkSync to be junction , else if not Windows 10 , use the normal dir type. Known issue has been commented on :

fs.symlink can’t create directory symlinks on Windows

Also using require("path").normalize(stringPath) seems to be much more robust and dynamic either using POSIX or Windows. And I run the test successfully after done all the fixes. Hope this is going to test on other OS as well to avoid other bugs. Let me know if you need me anything to fix on my code @boutell

@ammein
Copy link
Contributor Author

ammein commented Sep 14, 2019

It also needs to have attention when using Windows 10 for a unit test. The only thing that WORKS on Windows OS is to have root : module that will initialize on the current folder :

apos = require('apostrophe')({
            root : module,
            testModule: true,
            baseUrl: 'http://localhost:7780',
            modules: {
                'apostrophe-express': {
                    port: 7780
                },
            },
            afterInit: function (callback) {
                return callback(null);
            },
            afterListen: function (err) {
                assert(!err);
                done();
            }
        });

I believe this should start documented on HOWTO section on creating our own unit tests based on the OS platform. I am dreaming about making a HOWTO guideline on creating this unit tests for everyone's sake. This is a "Yahoooooo !" experience for me that it is finally working on Windows Platform that turns WTF behavior into an AWESOME experience. What do you think @boutell ?

@boutell
Copy link
Member

boutell commented Sep 15, 2019

Unless I miss my guess path.sep is not actually defined here, path was not required and assigned to a variable of that name (and I usually don't, because it is such a common variable name that requiring it at the top can lead to bugs; you can require it where you need it).

@ammein
Copy link
Contributor Author

ammein commented Sep 15, 2019

path.sep is actually a separator for file path. If POSIX, uses / , otherwise Windows uses \ . I thought using this path.sep will solve both platform or else, we could just use if else for that separator string. Anyway, path orginally defined on that file if I'm not mistaken. If not, I could use require("path").sep directly. Should I do it directly ?

@boutell
Copy link
Member

boutell commented Sep 15, 2019 via email

@ammein
Copy link
Contributor Author

ammein commented Sep 15, 2019

Done. I decided to use it directly to avoid incoming future bugs.

@boutell boutell merged commit 8cd0d31 into apostrophecms:master Sep 16, 2019
@boutell
Copy link
Member

boutell commented Sep 16, 2019

Thanks!

@ammein ammein deleted the test-regex branch September 16, 2019 15:41
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.

2 participants