jpitucha commented 5 years ago

Reference to related ticket: resolves #51 Description of changes: Makes build command work properly on Windows.

ghost commented 5 years ago

DeepCode analyzed this pull request. There is 1 new info report.

Click to see more details.

codecov[bot] commented 5 years ago

Codecov Report

Merging #165 into master will decrease coverage by 0.23%. The diff coverage is 25%.

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage    83.6%   83.36%   -0.24%     
==========================================
  Files          64       64              
  Lines        1049     1052       +3     
==========================================
  Hits          877      877              
- Misses        172      175       +3

Impacted Files	Coverage Δ
src/server/connectToDb.js	`75% <25%> (-25%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update be01451...98c1a0b. Read the comment docs.

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 1 new warning and 5 new info reports.

Click to see more details.

michaelKurowski commented 5 years ago

Things to check after finish: Prior to below tests run npm i in the git root directory

Does gulp prepareDev, filling config and gulp start results in a working LokIM instance?
Does gulp prepare, filling config and gulp start results in a working LokIM instance?
Does heroku works with the new build system?
Does generating docs works?
Does generating tests coverage reports works?
Does linting works?
Does cypress works?
Has README.md been updated accordingly?

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 1 new warning and 5 new info reports.

Click to see more details.

michaelKurowski commented 5 years ago

@obsceniczny remember to run git fetch origin and git pull --force since I've modified your branch directly

michaelKurowski commented 5 years ago

Let's add supported OSes later to Prerequisites tab in the readme

michaelKurowski commented 5 years ago

Why src/server/src/server/public/bundle.js and src/server/src/server/public/bundle.map.js have been commited?

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 1 new warning and 5 new info reports.

Click to see more details.

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 1 new warning and 5 new info reports.

Click to see more details.

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 1 new warning and 5 new info reports.

Click to see more details.

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 1 new warning and 5 new info reports.

Click to see more details.

michaelKurowski commented 5 years ago

@Jordan141 Could you please install gulp globally, and then run gulp installServerDependencies and tell us if it's slow? It takes like 30 min to install dependencies on Jacob's Windows, while it works flawless on Linux.

michaelKurowski commented 5 years ago

Ps please do not count Cypress installation, as it's slow regardless of the OS

jpitucha commented 5 years ago

@Jordan141 Could you please install gulp globally, and then run gulp installServerDependencies and tell us if it's slow? It takes like 30 min to install dependencies on Jacob's Windows, while it works flawless on Linux.

Linux.. on VM in addition

michaelKurowski commented 5 years ago

TODO Ensure that the coverage is being correctly reported

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 1 new warning and 6 new info reports.

Click to see more details.

Jordan141 commented 5 years ago

Testing on Windows

Steps to recreate

Navigate to root folder of Lokim.
Follow the instructions set above by Michael.

Installing gulp globally and performing requested tasks result in success

I am unable to recreate the 30 minute waiting time for server dependencies. My current machine utilizes an i7-4790k on an SSD, while I assume this would see faster times than a server, I don't see it being that much of an improvement. OS is the latest build of Windows 10.

michaelKurowski commented 5 years ago

Heroku is working. Although we can't remove heroku-postinstall script from the main package.json

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 1 new warning and 5 new info reports.

Click to see more details.

michaelKurowski commented 5 years ago

Ensure to fail gulp commands once receiving errors. i.e. ensure that failing tests crash CI

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 2 new info reports.

Click to see more details.

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 2 new info reports.

Click to see more details.

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 2 new info reports.

Click to see more details.

codecov[bot] commented 5 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@be01451). Click here to learn what that means. The diff coverage is 25%.

@@            Coverage Diff            @@
##             master     #165   +/-   ##
=========================================
  Coverage          ?   83.84%           
=========================================
  Files             ?       62           
  Lines             ?     1046           
  Branches          ?        0           
=========================================
  Hits              ?      877           
  Misses            ?      169           
  Partials          ?        0

Impacted Files	Coverage Δ
src/server/connectToDb.js	`75% <25%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update be01451...eb6e223. Read the comment docs.

ghost commented 5 years ago

DeepCode analyzed this pull request. There are 3 new info reports.

Click to see more details.

michaelKurowski commented 5 years ago

Working at this point

michaelKurowski commented 5 years ago

Looks like MongoDB container sometimes exits before backend tests run, which causes them to fail.

For coverage issues - i've ran it today and it reported ERROR: Coverage for lines (59.23%) does not meet global threshold (60%) instead of 0%.

Still investigating

michaelKurowski commented 5 years ago

The CI passed it might have been related to cache or dependencies. Remove node_modules in root, server and frontend

michaelKurowski commented 5 years ago

TODO We need to add caching for root node_modules in the CI

michaelKurowski commented 5 years ago

In order to test if the PR is working on your machine, follow below steps

--- peparing user environment ---

clone a fresh repo and checkout git checkout "#51-make-build-system-os-agnostic-jacob"
run npm i in the root git directory
run gulp prepare
you should see src/server/config.json fill it
run gulp start
you should see a lokIM instance up and running
kill LokIM

--- peparing dev environment ---

remove node_modules, src/server/node_modules and src/server/frontEnd/node_modules
run npm i in the root directory, and gulp prepareDev
fill config and run gulp start
you should see LokIM up and running
kill LokIM

--- generating documentation ---

run gulp generateDocs
you should see documentation for server generated in src/server/out

--- generating test coverage ---

run gulp serverTestCoverage
all tests should pass
you should see coverage inside src/server/coverage
run gulp frontEndTestCoverage
all tests should pass
you should see coverage inside src/server/frontEnd/coverage

--- linting ---

place a semicolon in backend` and frontend code
run gulp eslint
both semicolons should cause errors
run gulp eslintAutoFix
both semicolons should disappear

michaelKurowski commented 5 years ago

@Jordan141 Please test on Windows 10

@MarcinGrzeszczak Please test on Mac

@obsceniczny Please find a Windows 7 and test it there

I'll test on Linux

Guys, I know that the instruction is not short, but it's the only way to be sure, that we won't encounter any strange errors later during development. Besides, almost all steps will be incredibly fast to perform (except testing gulp preparing tasks).

michaelKurowski commented 5 years ago

@obsceniczny Tested Everything works on Linux

Jordan141 commented 5 years ago

Final Gulp Test

Installation

prepareDev - 54s on fresh install
prepare - 30s

Testing

serverTest - success
frontEndTest - success
serverTestCoverage - coverage wrongly reported
frontEndTestCoverage - success - 9.22s
testCypress - success

Linting

eslint - success 2.03s
eslintAutoFix - success - 4s

Generate Docs

generateDocs - success - 0.446s

michaelKurowski commented 5 years ago

https://github.com/istanbuljs/nyc/issues/1065 I've created an issue regarding the coverage issues on Windows 10 and 7

MarcinGrzeszczak commented 5 years ago

serverTestCoverage enters to lcov-report folder


File                                  |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
--------------------------------------|----------|----------|----------|----------|-------------------|
All files                             |    33.94 |    22.27 |    54.41 |    69.44 |                   |
 server                               |    81.05 |    84.27 |    83.78 |    83.15 |                   |
  configFileService.js                |      100 |    95.83 |      100 |      100 |                56 |
  connectToDb.js                      |    66.67 |       50 |    66.67 |       75 |          13,14,15 |
  dbConnectionProvider.js             |      100 |      100 |      100 |      100 |                   |
  dev.webpack.config.js               |        0 |      100 |      100 |        0 |             1,3,8 |
  index.js                            |        0 |      100 |        0 |        0 |     1,4,5,6,7,8,9 |
  init.js                             |      100 |       90 |      100 |      100 |                10 |
  initializationProcedures.js         |    69.44 |       80 |    72.73 |    73.53 |... ,52,53,107,108 |
  logger.js                           |    91.67 |       50 |      100 |      100 |                12 |
  prod.webpack.config.js              |        0 |      100 |      100 |        0 |             1,2,3 |
  utilities.js                        |    83.33 |    86.36 |       90 |    84.85 |    22,23,24,30,46 |
 server/coverage/lcov-report          |        0 |        0 |        0 |        0 |                   |
  block-navigation.js                 |        0 |        0 |        0 |        0 |... 53,54,58,59,63 |
  prettify.js                         |        0 |        0 |        0 |        0 |                 1 |
  sorter.js                           |        0 |        0 |        0 |        0 |... 52,153,154,158 |
 server/models                        |      100 |      100 |      100 |      100 |                   |
  user.js                             |      100 |      100 |      100 |      100 |                   |
 server/passport                      |      100 |      100 |      100 |      100 |                   |
  strategies.js                       |      100 |      100 |      100 |      100 |                   |
  strategyUtils.js                    |      100 |      100 |      100 |      100 |                   |
 server/routes                        |    93.75 |       50 |      100 |    93.75 |                   |
  router.js                           |    93.75 |       50 |      100 |    93.75 |                17 |
 server/routes/controllers            |    91.67 |      100 |    88.89 |    91.67 |                   |
  logInUser.js                        |    57.14 |      100 |       50 |    57.14 |             6,7,8 |
  logOutUser.js                       |      100 |      100 |      100 |      100 |                   |
  register.js                         |      100 |      100 |      100 |      100 |                   |
 server/routes/controllers/Middleware |      100 |      100 |      100 |      100 |                   |
  isUserAuthenticated.js              |      100 |      100 |      100 |      100 |                   |
 server/routes/controllers/utilities  |    82.35 |    66.67 |       75 |     87.5 |                   |
  initializeLoginHandler.js           |    76.92 |    66.67 |    66.67 |    83.33 |               5,7 |
  responseManager.js                  |      100 |      100 |      100 |      100 |                   |
 server/ws-routes                     |      100 |      100 |      100 |      100 |                   |
  ConnectionsRepository.js            |      100 |      100 |      100 |      100 |                   |
  ConnectionsRepositoryProvider.js    |      100 |      100 |      100 |      100 |                   |
  webSocketRouting.js                 |      100 |      100 |      100 |      100 |                   |
 server/ws-routes/controllers         |    95.31 |       50 |    89.47 |    96.67 |                   |
  Room.js                             |    97.87 |       75 |    92.86 |    97.78 |               116 |
  Users.js                            |    88.24 |       25 |       80 |    93.33 |                35 |
--------------------------------------|----------|----------|----------|----------|-------------------|```

MarcinGrzeszczak commented 5 years ago

Other steps went well on Mac

michaelKurowski commented 5 years ago

Windows issues fixed. @obsceniczny will push the fix.

michaelKurowski / lokim2

#51 Make LokIM build commands OS agnostic #165

Codecov Report

Testing on Windows

Codecov Report

Final Gulp Test

Installation

Testing

Linting

Generate Docs