Skip to content
This repository has been archived by the owner on Feb 13, 2019. It is now read-only.

Fixed broken Mocha test for aspnet:Config sub-generator #420

Closed
wants to merge 35 commits into from

Conversation

scottaddie
Copy link
Contributor

When running the npm test command, the Mocha test for the aspnet:Config sub-generator was broken and returned an error message. I tracked this error down and found that it was thrown from the yeoman-environment module's lib/environment.js file. Because there's a file named config.js in the root of the project, that file was being detected before the Config sub-generator. Since config.js isn't a generator, an error was thrown stating that the aspnet:Config sub-generator wasn't installed.

To fix this problem, I renamed the sub-generator to aspnet:ConfigJson. This change does a couple things:

  1. Makes the name consistent with other JSON file sub-generators, such as aspnet:BowerJson and aspnet:PackageJson
  2. Makes the sub-generator unique, which avoids file name collisions

Since I was fixing a problem with one of the unit tests, I updated the Mocha and Chai dependencies as well.

peterblazejewicz and others added 29 commits October 6, 2015 22:06
- add template and subgenerator
- add test coverage
- update documentation
…ibrary

⬆️ Update Class library for beta8. Closes OmniSharp#381
Address Beta8: Update Dockerfile subgenerator OmniSharp#395
- remove hosting.ini
- remove config.json
- add appsettings.json
- great beta8 renaming
- implementation changes
- update copy routine
- update test coverage
- add web.config file to project
Note: web.config file is probably needed when hosting in IIS
container
- remove hosting.ini from project
- rewrite kestrel/web commands
- update copy routine
- update test
- add web.config file
Note: the web.config file is probably needed for hosting
in IIS container
Note: the web.config is problaby required when hosting
your project on IIS container
…pplication

⬆️ Update Empty Application for beta8. Closes OmniSharp#382
…-application

⬆️ Update Console Application for beta8. Closes OmniSharp#380
…lication

⬆️ Update Web Application for beta8. Closes OmniSharp#385
…-web-app-basic

Add missing web.config to web basic template
Update project animation of CLI usage for beta8
⬆️ Update project deps for ConsoleApplication for beta8. Closes OmniSharp#408
@peterblazejewicz
Copy link
Member

  1. what was that error? I always run tests when reviewing PRs
  2. why rewriting all this, instead of renaming 'config.js' added as namespace-finding util module?
  3. what was that problem with deps requiring update? (we won't be able to revert deps update easily)
    Thanks!
  4. edit: I agree about some naming problems - but we don't know what are naming internals for MS template items yet (they still didn't released them)

@spboyer
Copy link
Contributor

spboyer commented Oct 15, 2015

@peterblazejewicz agreed, I am not getting any errors on aspnet:Config, not sure this PR is needed.

@scottaddie
Copy link
Contributor Author

@peterblazejewicz To answer your questions:

  1. Here's what I see when running npm test. I got the same results on 3 different machines. All 3 were running Node v4.2.0 with npm 3.3.8.
    test_fail_1
    test_fail_2
  2. I suppose I could've done something like that, but I felt that this change would enforce consistency. The other JSON file sub-generators I saw in the project were suffixed with "JSON".
  3. No problem at all with the Mocha and Chai dependencies. I just figured that I was in the code that used them, so this was the perfect opportunity to bring things up-to-date.

My question to @spboyer and @peterblazejewicz would be, what's different with your setups that you're not encountering this problem? Specifically, which versions of Node and npm are you using?

@peterblazejewicz
Copy link
Member

what's different with your setups that you're not encountering this problem? Specifically, which versions of Node and npm are you using?

OS X/Node 0.12.* because of this: #351, NPM 2.11.*
I'll try at work tomorrow on Windows OS

@spboyer
Copy link
Contributor

spboyer commented Oct 15, 2015

I'm running node 0.10.38 or 0.12.x and npm 2.14.x on OSX, and the same versions in WIN 10. I have not done any testing on npm 3.x or node 4. Before rc1 we need to put this into the test cycle to support node 4.x

@david-driscoll
Copy link
Member

I get the same error on Windows I just wound up ignoring it, because things
pass just fine on Travis.
On Thu, Oct 15, 2015 at 5:10 PM Shayne Boyer notifications@github.com
wrote:

I'm running node 0.10.38 or 0.12.x and npm 2.14.x on OSX, and the same
versions in WIN 10. I have not done any testing on npm 3.x or node 4.
Before rc1 we need to put this into the test cycle to support node 4.x


Reply to this email directly or view it on GitHub
#420 (comment)
.

Thanks,
David

@scottaddie
Copy link
Contributor Author

I saw this same error on Windows 7, 8.1, and 10.

@spboyer
Copy link
Contributor

spboyer commented Oct 15, 2015

I would suggest installing nvm and running node 0.10.x or 0.12.x with npm 3, this appears to be a compatibility issue with node 4

@scottaddie
Copy link
Contributor Author

In case anyone is interested, here's where the error is thrown:

environmentjs

Notice that the resolved property of the Generator object has the path to the project root's config.js file.

@peterblazejewicz
Copy link
Member

I haven't managed to recreate this on OS X, so this could be Windows only issue (see @david-driscoll comment above).
@scottaddie
Scott, if still not convinced to rename only config.js, could you consider to splitting PR into separate one, so update to deps is in different PR?

Reverted Chai and Mocha version updates. To be included in separate PR.
Reverted Chai and Mocha version updates. To be included in separate PR.
@scottaddie
Copy link
Contributor Author

@peterblazejewicz I messed something up in my local repo while trying to revert some changes. I'm going to close this PR and resubmit this as 2 PRs: 1 for the new deps and 1 for the config.js name change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants