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

Minor js fix, makes two files in test suite valid JavaScript #990

Merged
merged 1 commit into from Nov 30, 2012
Merged

Minor js fix, makes two files in test suite valid JavaScript #990

merged 1 commit into from Nov 30, 2012

Conversation

beeman
Copy link
Contributor

@beeman beeman commented Nov 28, 2012

Makes sure these two files are valid JavaScript by adding comment marks to them, this way editors (NetBeans 7.3 for instance) stop marking them invalid.

Screenshot of my editor with one of these files: http://i.imgur.com/e91fk.png

to them, this way editors (NetBeans 7.3 for instance) stop marking them
invalid.
@lorenzo
Copy link
Member

lorenzo commented Nov 28, 2012

Did you run the test suite?

@beeman
Copy link
Contributor Author

beeman commented Nov 28, 2012

I did, the output is more or less similar (all, but the Assertion count):

Tests: 3224, Assertions: 17166, Failures: 36, Errors: 10, Skipped: 132.

As far as I can tell from the tests both these files are not syntactically invalid .js files on purpose, so there is no harm in making them valid, or do I miss something here? :-)

@lorenzo
Copy link
Member

lorenzo commented Nov 28, 2012

More or less similar?! you have 36 failures and 10 errors haha

@lorenzo
Copy link
Member

lorenzo commented Nov 28, 2012

Just to let you know, you should run the tests with --stderr

@beeman
Copy link
Contributor Author

beeman commented Nov 28, 2012

What I meant is that my change to the code did not change the amount of failures and errors in the test :-)

I'll run it with --stderr to see if that makes a difference :-)

@lorenzo
Copy link
Member

lorenzo commented Nov 28, 2012

Well, your changes will break some tests. Those files are in the test
folder because we do assertions of their contents.

On Thu, Nov 29, 2012 at 12:38 AM, Bram Borggreve
notifications@github.comwrote:

What I meant is that my change to the code did not change the amount of
failures and errors in the test :-)

I'll run it with --stderr to see if that makes a difference :-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/990#issuecomment-10828425.

@beeman
Copy link
Contributor Author

beeman commented Nov 28, 2012

Oke, I could not found it...

I just ran the test with --stderr and now it just gives one failure and error, but that is with and without my changes...

Tests: 3224, Assertions: 18492, Failures: 1, Errors: 1, Skipped: 132.

@lorenzo
Copy link
Member

lorenzo commented Nov 28, 2012

Then most probably those files are not even needed anymore, will investigate

On Thu, Nov 29, 2012 at 12:47 AM, Bram Borggreve
notifications@github.comwrote:

Oke, I could not found it...

I just ran the test with --stderr and now it just gives one failure and
error, but that is with and without my changes...

Tests: 3224, Assertions: 18492, Failures: 1, Errors: 1, Skipped: 132.


Reply to this email directly or view it on GitHubhttps://github.com//pull/990#issuecomment-10828700.

@beeman
Copy link
Contributor Author

beeman commented Nov 28, 2012

Thank you very much, also for your help on testing 👍

@markstory
Copy link
Member

I just applied these changes locally, and ran all the tests. No tests failed, for me so I'm going to merge this and deal with any fallout that happens in the CI builds.

markstory added a commit that referenced this pull request Nov 30, 2012
Minor js fix, makes two files in test suite valid JavaScript
@markstory markstory merged commit 68976a0 into cakephp:2.3 Nov 30, 2012
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.

None yet

3 participants