Set correct mime-type for js files (should be application/javascript) - fixes #4496

[acoulton]

A very small change to fix http://dev.kohanaframework.org/issues/4496 - File::mime_by_ext reports an invalid mime-type (application/x-javascript) for .js files which is not valid per the RFC.

[Kohana-Builds]

Build Scheduled

[Kohana-Builds]

Build Passed
http://ci.kohanaframework.org/job/kohana-3.2-pull/95/

[shadowhand]

Please create a unit test for this.

[acoulton]

No problem, but just wanted to check what/how you want testing.

As far as I can see in https://github.com/kohana/core/blob/3.2/develop/tests/kohana/FileTest.php File::mime_by_ext currently isn't covered at all and File::mime's test just validates that it returns a valid mime-type rather than asserting a specific result (in fact, from the doc-comments looks like it was refactored to do that as the comments still refer to a $expected parameter which is no longer present).

Should I pick maybe this, something without an extension and something where the extension maps to an array of types and test against specific assertions? Or do you more want a ticket test just for this specific type? Presumably a full test matching everything in the mime_types config would be overkill.

Just want to be sure my test matches your strategy for these classes.

[acoulton]

@shadowhand, not sure if you saw my reply above- if you can give me a pointer on how you'd like to see this patch for File::mime_by_ext tested (given the method isnt currently tested at all) I'll get that implemented.

[shadowhand]

This should be pretty easy to test, all you need to do is call File::mime_by_ext on a filename and verify the result.

[acoulton]

@shadowhand, thanks I understand how to write a test for this. As above,
what I was asking is do you want me to just test this specific type -
File::mime_by_ext is currently not tested at all?

That will obviously test this specific change but will give very limited
coverage of the method in general and I wasn't sure if you were asking me
to produce something wider in scope eg with a couple of examples.

On 20 May 2012 22:49, "Woody Gilk" <
reply@reply.github.com>
wrote:

This should be pretty easy to test, all you need to do is call
File::mime_by_ext on a filename and verify the result.

---

Reply to this email directly or view it on GitHub:
#239 (comment)

[shadowhand]

You will have to create the unit test file yourself. As long as this method is tested, we can accept the pull request. If you want to create tests for other methods, even better!

[acoulton]

@shadowhand obviously I'll add the test to Kohana_FileTest rather than add
a new file.

I'm either being really unclear, or you're not actually reading what I'm
writing. So one last attempt to get clarity.

Basically, do you want me to explicitly test the return value is
application/javascript when passed .js - this being inconsistent with the
test for File::mime which seems like it has been rewritten at some point to
just assert that any valid mime type is returned but not what. Obviously if
I just test for a valid type as in that test, it would pass before or after
this commit.

In other words, should the test of the File::mime_by_ext method be coupled
to the values in the mimes config file or not?

It also would seem bizarre to write a test just for this one input/output
combination on a method with several logic flows, so I do want to produce a
more general test case (with a sample of possible input filenames params)
for the method.
On 21 May 2012 04:18, "Woody Gilk" <
reply@reply.github.com>
wrote:

You will have to create the unit test file yourself. As long as this
method is tested, we can accept the pull request. If you want to create
tests for other methods, even better!

---

Reply to this email directly or view it on GitHub:
#239 (comment)

[shadowhand]

@acoulton After some thought, I don't think it makes sense to unit test this, as all the values to test against come from configuration files. If the user has a different MIME preference, unit tests will start failing. I'm going to merge this as-is.

[shadowhand]

Merged up to 3.3/develop in 9233fd0

[acoulton]

Great, thanks @shadowhand.

On Thu, May 24, 2012 at 6:59 AM, Woody Gilk <
reply@reply.github.com

wrote:

Merged up to 3.3/develop in 9233fd0

---

Reply to this email directly or view it on GitHub:
#239 (comment)