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

Initial commit for #5, with a single basic e2e test #20

Merged
merged 7 commits into from
Aug 26, 2015

Conversation

tjaffri
Copy link
Contributor

@tjaffri tjaffri commented Aug 25, 2015

This is the first of hopefully a few commits. Some points to note:

  1. I tried to structure the e2e tests similar to the unit tests, i.e. under the test/e2e structure.
  2. I use typing files to make it easier to author tests when using an IDE that uses typing files (e.g. VS code). This helps with intesense and discovering what you are testing. I excluded the typings via .gitignore in case you don't want them in the repo. Let me know if you do, however, and it's easy for me to add it in. Personally, I do consider it a best practice of the sort you are trying to highlight in this project.
  3. I added a wiki article in your main wiki at https://github.com/leob/ionic-quickstarter/wiki/E2E-Testing. Rather than put all this in the README.md, I thought I would start with a wiki. Later, when this is working better, we can perhaps add a small section in the README too.
  4. I didn't put too much thought yet into dev vs test vs other build flavors. Right now this is pretty basic and I wanted to get some quick feedback before I did more.

@leob
Copy link
Owner

leob commented Aug 25, 2015

Thanks, I'm going to have a look. I have no experience with typing files (I saw that it's Typescript). I'm using IntelliJ/Webstorm as my IDE, don't know if that works with typing files.

Is it extra work to maintain the typing files? As far as I could see it's just something that you drop in. Anyway I'll first look at the rest of the PR and then we'll look at the typing files, if it has benefits then I'd say we can drop it in.

@leob leob merged commit a5d5608 into leob:master Aug 26, 2015
leob added a commit that referenced this pull request Aug 26, 2015
@leob
Copy link
Owner

leob commented Aug 26, 2015

Okay cool!

Reviewed/tested your PR, and merged it.

The e2e tests work like a charm, I love it.

Also the Wiki docs are fine, they're very clear. In the README I just added a link to the Wiki section.
No need to copy all that info into the README, the link there pointing to the Wiki is fine.

@leob
Copy link
Owner

leob commented Aug 26, 2015

P.S. by the way I've just made a tiny change regarding the CSS class used for the "form-errors" directive (also had to modify the e2e test accordingly).

Please sync your local repo with the upstream repo, e.g:

git fetch upstream
git checkout master
git merge upstream/master

and so on.

leob added a commit that referenced this pull request Nov 10, 2016
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