[test] Remove skip on Edge

[m4theushw]

Follow-up from #1665 (review). The reason to skip these tests is explained in #1665 (comment).

[oliviertassinari]

I had a quick look at the issue. This diff seems to be enough:

diff --git a/packages/grid/data-grid/src/tests/filtering.DataGrid.test.tsx b/packages/grid/data-grid/src/tests/filtering.DataGrid.test.tsx
index 38cb88af..8ef1ae66 100644
--- a/packages/grid/data-grid/src/tests/filtering.DataGrid.test.tsx
+++ b/packages/grid/data-grid/src/tests/filtering.DataGrid.test.tsx
@@ -36,7 +36,7 @@ describe('<DataGrid /> - Filter', () => {
     rows?: any[];
     columns?: any[];
     operator?: string;
-    value?: string;
+    value?: any;
     field?: string;
   }) => {
     const { operator, value, rows, columns, field = 'brand' } = props;
@@ -187,14 +187,7 @@ describe('<DataGrid /> - Filter', () => {
     });
   });

-  describe('Date operators', function test() {
-    const isEdge = /Edg/.test(window.navigator.userAgent);
-    before(function before() {
-      if (isEdge) {
-        // We need to skip edge as it does not handle the date the same way as other browsers.
-        this.skip();
-      }
-    });
+  describe('Date operators', () => {
     [
       { operator: 'is', value: new Date(2000, 11, 1), expected: ['12/1/2000'] },
       { operator: 'not', value: new Date(2000, 11, 1), expected: ['1/1/2001', '1/1/2002'] },
@@ -203,10 +196,12 @@ describe('<DataGrid /> - Filter', () => {
       { operator: 'before', value: new Date(2001, 0, 1), expected: ['12/1/2000'] },
       { operator: 'onOrBefore', value: new Date(2001, 0, 1), expected: ['12/1/2000', '1/1/2001'] },
     ].forEach(({ operator, value, expected }) => {
+      const formatter = new Intl.DateTimeFormat('en');
+
       it(`should allow object as value and work with valueGetter, operator: ${operator}`, function dateOpsTest() {
         render(
           <TestCase
-            value={value.toLocaleDateString()}
+            value={value}
             operator={operator}
             rows={[
               {
@@ -222,7 +217,14 @@ describe('<DataGrid /> - Filter', () => {
                 brand: { date: new Date(2002, 0, 1) },
               },
             ]}
-            columns={[{ field: 'brand', valueGetter: (params) => params.value.date, type: 'date' }]}
+            columns={[
+              {
+                field: 'brand',
+                valueGetter: (params) => params.value.date,
+                valueFormatter: (params) => formatter.format(params.value),
+                type: 'date',
+              },
+            ]}
           />,
         );
         expect(getColumnValues()).to.deep.equal(expected.map((res) => res));

[oliviertassinari]

We can't use new Date('') per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date#parameters

[m4theushw]

Much better. 👍

[dtassone]

should we use this in the date column definition 🤔 ?

[oliviertassinari]

As far as I know (new Intl.DateTimeFormat('en')).format(params.value) is equivalent to params.value.toLocaleDateString('en') (but performance might be different). The different with the date column definition is the introduction of a default locale. Edge was using something else than en-US in BrowserStack, so we needed to force a unique locale.

Actually, we could do to reduce ambiguity or use toLocaleDateString to reduce confusion:

Suggested change

	const formatter = new Intl.DateTimeFormat('en');
	const formatter = new Intl.DateTimeFormat('en-US');

[m4theushw]

I changed to params.value.toLocaleDateString('en-US') because it's simpler to use.

I couldn't find any difference between the two implementations, they even accept the same arguments. In a small performance test I did, toLocaleDateString ran slightly faster, so I would keep it. https://jsbench.me/l2kowy4klg/3

[dtassone]

why not integrate the locale detection in the date col def format function, and do the same for numbers?

[oliviertassinari]

From what I understand, we want to use the default locale of the end-users but use the same locale for all the test environments. Are you saying that we should fork in the source and detect the test environment?
Maybe there is another way to force the locale on BrowserStack 🤷‍♂️

[dtassone]

How about we use the locale used in the translation to actually format the dates and numbers?

In tests, we will have to either override the date formatter to force a pre-defined locale, or override the locale detector...
but yes in tests we would need a static locale.

[oliviertassinari]

How about we use the locale used in the translation to actually format the dates and numbers?

We talk about this problem in mui/material-ui#24495. It's the same problem for the formatting of numbers.

[oliviertassinari]

Approving as it's already a good step forward, so we don't ignore Edge in the tests.

For the next step, I find that forcing the locale to en-US during the tests is pragmatic. Maybe something like:

diff --git a/packages/grid/_modules_/grid/models/colDef/gridDateColDef.ts b/packages/grid/_modules_/grid/models/colDef/gridDateColDef.ts
index b0bc947a..274d551c 100644
--- a/packages/grid/_modules_/grid/models/colDef/gridDateColDef.ts
+++ b/packages/grid/_modules_/grid/models/colDef/gridDateColDef.ts
@@ -5,9 +5,11 @@ import { getGridDateOperators } from './gridDateOperators';
 import { GRID_STRING_COL_DEF } from './gridStringColDef';
 import { GridColTypeDef } from './gridColDef';

+// Force the locale to a deterministic value in the tests
+const defaultLocale = process.env.NODE_ENV === 'test' ? 'en-US' : undefined;
+
 export function gridDateFormatter({ value }: { value: GridCellValue }) {
   if (isDate(value)) {
-    return value.toLocaleDateString();
+    return value.toLocaleDateString(defaultLocale);
   }
   return value;
 }

but no real preferences.

[oliviertassinari]

Actually, @m4theushw you were right, this shouldn't be a Date object but a string. Otherwise, we are off the actual behavior of the component.

[m4theushw]

I returned the string value in #1722 to match the actual implementation.

[oliviertassinari]

Suggested change

	value?: any;
	value?: string;